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

Mon May 20 07:52:30 PDT 2019

gribozavr added inline comments.

================
Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:78
+      /// The DirectoryWatcher that originated this event is no longer valid and
+      /// it's behavior is undefined.
+      /// The prime case is kernel signalling to OS-specific implementation of
----------------
"its"

================
Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:78
+      /// The DirectoryWatcher that originated this event is no longer valid and
+      /// it's behavior is undefined.
+      /// The prime case is kernel signalling to OS-specific implementation of
----------------
gribozavr wrote:
> "its"
I'd say "unspecified".  "Undefined behavior" has a specific meaning in C++, and I don't believe we have that.

Everywhere in the patch.

================
Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:79
+      /// it's behavior is undefined.
+      /// The prime case is kernel signalling to OS-specific implementation of
+      /// DirectoryWatcher some resource limit being hit.
----------------
Please add blank lines between paragraphs (everywhere in the patch).

================
Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:95
+    EventKind Kind;
+    /// Filename - a relative path to the watched directory or empty string in
+    /// case event is related to the directory itself.
----------------
Don't repeat field names in comments.

================
Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:95
+    EventKind Kind;
+    /// Filename - a relative path to the watched directory or empty string in
+    /// case event is related to the directory itself.
----------------
gribozavr wrote:
> Don't repeat field names in comments.
"a relative path" -- relative to what?

================
Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:95
+    EventKind Kind;
+    /// Filename - a relative path to the watched directory or empty string in
+    /// case event is related to the directory itself.
----------------
gribozavr wrote:
> gribozavr wrote:
> > Don't repeat field names in comments.
> "a relative path" -- relative to what?
Is it really a path to the directory?

================
Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:103
+
+  typedef std::function<void(llvm::ArrayRef<Event> Events, bool isInitial)>
+      EventReceiver;
----------------
"IsInitial"

================
Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:103
+
+  typedef std::function<void(llvm::ArrayRef<Event> Events, bool isInitial)>
+      EventReceiver;
----------------
gribozavr wrote:
> "IsInitial"
Users are not going to use this typedef in their code, so I feel like it is obfuscating code -- could you expand it in places where it is used?

================
Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:123
+                       DirectoryWatcher::EventReceiver Receiver,
+                       bool WaitInitialSync, std::string &Error);
+
----------------
Make it a static function in the DirectoryWatcher?

================
Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:86
+  EventQueue Queue;
+  // flag used for stopping all async acions
+  std::atomic<bool> Stop;
----------------
Don't write what something is used for (it can change), write what something is.

"If true, all async actions are requested to stop."

================
Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:106
+      FinishedInitScan(false), Receiver(Receiver),
+      InotifyPollingThread([this, InotifyFD]() {
+        // We want to be able to read ~30 events at once even in the worst case
----------------
I think this is too much code in the initializer list.  Could you move the body of the lambda into a method, and call it from this lambda?

================
Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:110
+        constexpr size_t EventBufferLength =
+            30 * (sizeof(struct inotify_event) + NAME_MAX + 1);
+        // http://man7.org/linux/man-pages/man7/inotify.7.html
----------------
NAME_MAX is 255, add some for inotify_event, and multiply by 30... we get almost 8 Kb on the stack. Should we allocate on the heap instead?

================
Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:112
+        // http://man7.org/linux/man-pages/man7/inotify.7.html
+        /* Some systems cannot read integer variables if they are not
+        properly aligned. On other systems, incorrect alignment may
----------------
Please use "//" comments.

================
Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:118
+        char Buf[EventBufferLength]
+            __attribute__((aligned(__alignof__(struct inotify_event))));
+
----------------
Please use AlignedCharArray from llvm/include/llvm/Support/AlignOf.h

================
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!
----------------
What is the role of the timeout and why does it need to be so small?

================
Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:210
+      ReceivingThread([this, WatchedDirPath]() {
+        // Initial scan of watched directory first ...
+        this->Receiver(getAsFileEvents(scanDirectory(WatchedDirPath)),
----------------
I think this is too much code in the initializer list. Could you move the body of the lambda into a method, and call it from this lambda?

================
Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:225
+            GotEvent = true;
+          };
+          if (!GotEvent) {
----------------
No need for the semicolon.

================
Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:271
+
+  return std::unique_ptr<clang::DirectoryWatcher>(new DirectoryWatcherLinux(
+      Path, Receiver, WaitInitialSync, InotifyFD, InotifyWD));
----------------
Use llvm::make_unique (and let `unique_ptr` do an implicit conversion to base).

================
Comment at: clang/unittests/DirectoryWatcher/CMakeLists.txt:16
+if(APPLE)
+  check_include_files("CoreServices/CoreServices.h" HAVE_CORESERVICES_H)
+  if(HAVE_CORESERVICES_H)
----------------
No need to check.  macOS will always have this file.  If it is not there, it is a big issue anyway.

================
Comment at: clang/unittests/DirectoryWatcher/CMakeLists.txt:20
+    set_property(TARGET DirectoryWatcherTests APPEND_STRING PROPERTY
+                 LINK_FLAGS ${DWT_LINK_FLAGS})
+  endif()
----------------
... LINK_FLAGS "-framework CoreServices"

No need for an intermediate variable.

================
Comment at: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:42
+  static bool
+  AreExpectedPresent(const std::vector<DirectoryWatcher::Event> &actual,
+                     const std::vector<DirectoryWatcher::Event> &expected) {
----------------
ContainsEvents

================
Comment at: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:73
+  // Fool-proof way how to compare.
+  bool AreExpectedPresentInInitial(
+      const std::vector<DirectoryWatcher::Event> &expected) {
----------------
"ContainsInitialEvents"

================
Comment at: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:80
+  // Fool-proof way how to compare.
+  bool AreExpectedPresentInNonInitial(
+      const std::vector<DirectoryWatcher::Event> &expected) {
----------------
"ContainsNonInitialEvents"

================
Comment at: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:131
+  // the async event handling picks them up. Can make this test flaky.
+  std::this_thread::sleep_for(std::chrono::milliseconds(100)); // 0.1s
+}
----------------
I'm certain this sleep will be flaky on heavily-loaded CI machines.  If you are going to leave it as a sleep, please make it 1s.  But is there really no better way?

================
Comment at: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:357
+
+  EXPECT_TRUE(eventConsumer.AreExpectedPresentInNonInitial(
+      {{DirectoryWatcher::Event::EventKind::WatcherGotInvalidated, ""}}));
----------------
I would strongly prefer if you used the gmock matchers (like Contains); as written, when the test fails, the only error we would get would be like "expected: true, actual: false".

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

https://reviews.llvm.org/D58418