-
Notifications
You must be signed in to change notification settings - Fork 247
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
Implement windows lockfile #1662
Conversation
841c869
to
5130df0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
i only have one comment/suggestion here and its not a showstopper imo |
5130df0
to
f4f3071
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! LGTM
2ad2bca
to
aa670bc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now a full review.
I think the unlockHandle
and the test parts need addressing; the rest is completely non-blocking subjective aesthetics.
@mtrmac I've made some updates based on your comments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
LGTM, there are just a few more opportunities to use the new enum instead of a bool
.
rwMutex: &sync.RWMutex{}, | ||
stateMutex: &sync.Mutex{}, | ||
lw: newLastWrite(), // For compatibility, the first call of .Modified() will always report a change. | ||
lockType: lType, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self:
- Isn’t this field only used if
locked
? So it doesn’t actually need to be set here. - Actually
locked
andlockType
could be combined.
Something to clean up later…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Actually
locked
andlockType
could be combined.
On second thought, that might not be worthwhile, e.g. some of the lock
functions would need to deal with the “unlocked” value.
@mikenorgate looks like you need to rebase this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks!
cfa60ea
to
af8b6db
Compare
Signed-off-by: Mike Norgate <[email protected]>
af8b6db
to
1ad1a4e
Compare
Thanks again! |
This is an implementation of
lockfile
for Windows, it includes a refactor of the Linux code to remove duplicationSigned-off-by: Mike Norgate [email protected]