jkorous marked 18 inline comments as done.
jkorous added inline comments.
================
Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:116
+
+ DirectoryWatcher::EventKind K = DirectoryWatcher::EventKind::Added;
+ if (ievt->mask & IN_MODIFY) {
----------------
jkorous wrote:
> mgorny wrote:
> > I don't think I understand this assumption. Have you any specific event in
> > mind that is supposed to be treated as added here? If not, maybe it'd be
> > better to have no default and instead assert that one of the conditions
> > below matched.
> SGTM. Will rewrite to Optional<> and assert.
Reconsidered and replaced with one-off closure in order not to complicate
following code with Optional.
================
Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:135
+ if (!statusOpt.hasValue())
+ K = DirectoryWatcher::EventKind::Removed;
+ }
----------------
akyrtzi wrote:
> mgorny wrote:
> > akyrtzi wrote:
> > > mgorny wrote:
> > > > akyrtzi wrote:
> > > > > 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 ?
> > > > Given the code is supposed to wrap filesystem notification layer, I'd
> > > > say it should pass the events unmodified and not double-guess what the
> > > > client expects. The client needs to be prepared for non-atomicity of
> > > > this anyway.
> > > So it should report an 'added' event but with optional mod-time,
> > > essentially reporting that something was added that doesn't exist ?
> > > I'd prefer not to do that because it complicates the client without any
> > > real benefit. It was great that you pointed out this part of the code but
> > > I'd recommend that if the file is gone we should ignore the 'added'
> > > event, instead of complicating the API for a corner case.
> > Except that is technically impossible to avoid reporting something that
> > doesn't exist because it can be removed just after you check for it. So the
> > client needs to *always* support it, otherwise it's fragile to race
> > conditions.
> >
> > This extra check just covers the short period (-> 0) between reporting and
> > checking. It's needless complexity that doesn't solve the problem. If it
> > does anything, then it gives you false security that you've solved the
> > problem when actually the file may still disappear 1 ns after you've
> > checked that it existed.
> Ok, that's fair, @jkorous I'm fine with removing the mod-time from the
> DirectoryWatcher API. We can get and report the mod-time at the index-store
> library layer.
Removed the conditional event kind change and removed ModTime.
================
Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:154
+ errorMsg += llvm::sys::StrError();
+ return true;
+ };
----------------
dexonsmith wrote:
> jkorous wrote:
> > mgorny wrote:
> > > The return values seem to be confusing. Any reason not to use `true` for
> > > success?
> > It seems there's no real consensus * about error/success mapping to bool
> > and since this is just implementation detail (not a public interface) I
> > think it's fine. `llvm::Error` has better semantics but seems a bit
> > heavyweight for this.
> >
> > * Just tried this: grep -nr "returns true" clang | grep -i error
> Agreed that much of LLVM/Clang uses the C convention where `false`/`0` is
> success.
>
> However, I'm curious whether it might be better to thread `llvm::Error`
> (and/or `llvm::Expected`) through, since you bring it up. They provide nice
> assertions when the caller forgets to check the result, they're cheap on
> success, and there's no true/false ambiguity.
Using `llvm::Error` machinery sounds like a good idea. I'll leave it for a
separate patch though.
================
Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:94
+ std::shared_ptr<EventQueue> evtQueue) {
+#define EVT_BUF_LEN (30 * (sizeof(struct inotify_event) + NAME_MAX + 1))
+ char buf[EVT_BUF_LEN] __attribute__((aligned(8)));
----------------
dexonsmith wrote:
> Can this be `constexpr int EventBufferLength` instead of `#define
> EVT_BUF_LEN`?
Yes, that's a good catch.
================
Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:196
+ while (true) {
+ if (close(inotifyFD) == -1 && errno == EINTR)
+ continue;
----------------
mgorny wrote:
> There's some fancy function for this in LLVM. `RetryAfterSignal`, I think.
Didn't know that one. Thanks!
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
