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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:48
+      return llvm::None;
+    } else {
+      DirectoryWatcher::Event Front = Events.front();
----------------
nit: get rid of the else


================
Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:156
+
+    for (char *p = Buf; p < Buf + NumRead;) {
+      if (p + sizeof(struct inotify_event) > Buf + NumRead) {
----------------
naming, it should be capital `P`


================
Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:211
+  // ... inotify-originated events processing ever after.
+  while (true) {
+    bool GotEvent = false;
----------------
`while (!Stop)` ?


================
Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:211
+  // ... inotify-originated events processing ever after.
+  while (true) {
+    bool GotEvent = false;
----------------
kadircet wrote:
> `while (!Stop)` ?
why the loop always tries to empty the queue? what would be broken if we simply stopped if `Stop` was set?

Also if that is important current implementation doesn't seem to be achiving that, since some events can be pushed to the queue after while loop exits.


================
Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:223
+        break;
+      std::this_thread::sleep_for(std::chrono::milliseconds(1));
+    }
----------------
maybe add a condition variable for queue state and waint on it?


================
Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:244
+      ReceivingThread([this]() { EventReceivingLoop(); }) {
+  while (WaitForInitialSync && !Stop && !FinishedInitScan) {
+    std::this_thread::sleep_for(std::chrono::milliseconds(1));
----------------
why not use two condition variables for stop and finishedinitscan?


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

https://reviews.llvm.org/D58418