akyrtzi added inline comments.
================
Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:135
+ if (!statusOpt.hasValue())
+ K = DirectoryWatcher::EventKind::Removed;
+ }
----------------
mgorny wrote:
> akyrtzi wrote:
> > mgorny wrote:
> > > jkorous wrote:
> > > > mgorny wrote:
> > > > > Why? I suppose this deserves a comment.
> > > > I'll add this comment:
> > > >
> > > > // The file might have been removed just after we received the event.
> > > Wouldn't that cause removals to be reported twice?
> > Not quite sure if it can happen in practice but I'd suggest to accept this
> > as potential occurrence and add it to documentation ("a 'removed' event may
> > be reported twice). I think it is better to handle a definite "fact" (the
> > file doesn't exist) than ignore it and assume the 'removed' event will
> > eventually show up, or try to eliminate the double reporting which would
> > over-complicate the implementation.
> >
> > After all, if inotify() is not 100% reliable then there's already the
> > possibility that you'll get a 'removed' event for a file that was not
> > reported as 'added' before.
> I see this as a partial workaround for race condition. You 'fix' it if
> removal happens between inotify reporting the event and you returning it. You
> don't if removal happens after you push it to events. Therefore, the caller
> still needs to account for 'added' event being obsolete. I don't really see a
> purpose in trying to workaround the problem partially here if the caller
> still needs to account for it. Effectively, you're replacing one normal case
> and one corner case with one normal case and two corner cases.
I was mainly pointing out that the client already has to be prepared for a
'removed' event that does not have an associated 'added' event, regardless of
what this code is doing. Therefore a potential double 'removed' event doesn't
add complexity to the client.
Could you clarify how the code should handle the inability to get the mod time
? Should it ignore the event ?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58418/new/
https://reviews.llvm.org/D58418
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
