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

[Michał Górny via Phabricator via cfe-commits]

mgorny added a comment.

Also, don't forget that `inotify()` is not 100% reliable on Linux, and it can miss events under high loads. So technically you should probably include periodic directory scans if you are going to rely on this actually reporting every file added.



================
Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:101
+    if (numRead == -1) {
+      return; // watcher is stopped.
+    }
----------------
Looks like you're not handling `EINTR`.


================
Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:106
+    for (char *p = buf; p < buf + numRead;) {
+      struct inotify_event *ievt = (struct inotify_event *)p;
+      p += sizeof(struct inotify_event) + ievt->len;
----------------
Maybe use C++ casts.

Also, I'd add some asserts for whether you've got enough data read.


================
Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:116
+
+      DirectoryWatcher::EventKind K = DirectoryWatcher::EventKind::Added;
+      if (ievt->mask & IN_MODIFY) {
----------------
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.


================
Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:135
+        if (!statusOpt.hasValue())
+          K = DirectoryWatcher::EventKind::Removed;
+      }
----------------
Why? I suppose this deserves a comment.


================
Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:154
+    errorMsg += llvm::sys::StrError();
+    return true;
+  };
----------------
The return values seem to be confusing. Any reason not to use `true` for success?


================
Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:193
+    return;
+  close(inotifyFD);
+  inotifyFD = -1;
----------------
You're ignoring return value.


================
Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher.cpp:95
+
+#if !defined(__has_include)
+#define __has_include(x) 0
----------------
Why not use CMake checks? You're already partially using them for frameworks.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58418/new/

https://reviews.llvm.org/D58418