Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implemented -[NSConditionLock lockWhenCondition:beforeDate:] #2508

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

triplef
Copy link
Contributor

@triplef triplef commented Apr 13, 2017

Fixes #845.


This change is Reviewable

Copy link

@DHowett-MSFT DHowett-MSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @triplef!
I'd like to see a few comments on the time manipulation, and perhaps some diversity in naming between t and tv (since they're close enough but totally different types.)
I wouldn't hold up the approval for that, though!

Note to merger: reword in imperative <=72 before merge

Copy link

@DHowett-MSFT DHowett-MSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've thought about this a bit more, and it would be great if you'd add some unit tests to the existing NSConditionLock tests for this!

*/
- (BOOL)lockWhenCondition:(NSInteger)condition beforeDate:(NSDate*)date {
UNIMPLEMENTED();
return NO;
int rc;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume rc is for "return code"? Please don't choose single letter / abbreviated variable names as it is unclear to future readers what the abbreviation / implicit word is. Perhaps statusCode / returnCode, etc here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DHowett-MSFT I didn't see the surrounding context sorry. Probably ok then.

@DHowett-MSFT
Copy link

@bbowman The rest of this file uses rc by convention, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants