[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

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