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

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.


================
Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:136
+
+            const int PollResult = poll(&PollReq, 1, TimeoutMs);
+            // There are inotify events waiting to be read!
----------------
jkorous wrote:
> gribozavr wrote:
> > What is the role of the timeout and why does it need to be so small?
> The whole idea is that we can't block on `read()` if we ever want to stop watching the directory, release resources (file descriptors, threads) and correctly destruct the DirectoryWatcher instance either
> - because of a bug in some other thread in the implementation
> - or asynchronous client action (e. g. destructor being called) in main application thread
> 
> The timeout adds latency in those scenarios.
Waking up 1000 times a second is not great -- it will lead to battery drain on laptops etc.

Please see https://stackoverflow.com/questions/8593004/waiting-on-a-condition-pthread-cond-wait-and-a-socket-change-select-simultan for non-busy-wait options.


================
Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:111
+
+  std::unique_ptr<llvm::AlignedCharArray<__alignof__(struct inotify_event),
+                                         EventBufferLength>>
----------------
"alignof()" expression is standard C++ since C++11. (No need for underscores.)


================
Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:114
+      ManagedBuffer = llvm::make_unique<llvm::AlignedCharArray<
+          __alignof__(struct inotify_event), EventBufferLength>>();
+  char *const Buf = ManagedBuffer->buffer;
----------------
use 'auto' to store the return value of make_unique?


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

https://reviews.llvm.org/D58418