Skip to content
This repository was archived by the owner on May 28, 2025. It is now read-only.

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 subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants