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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to