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

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added a comment.

Very nice testing approach!



================
Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:20
+/// Provides notifications for file changes in a directory.
+
+/// Invokes client-provided function on every filesystem event in the watched
----------------
Looks like triple slashes on empty lines got removed, splitting the doc comment.


================
Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:40
+// FDRead.
+// Currently used just for one-off termination signal.
+struct SemaphorPipe {
----------------
Three slashes for doc comments?


================
Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:40
+// FDRead.
+// Currently used just for one-off termination signal.
+struct SemaphorPipe {
----------------
gribozavr wrote:
> Three slashes for doc comments?
Don't write what something is used for currently, such comments go out of date quickly.  (Just delete the last sentence.)


================
Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:41
+// Currently used just for one-off termination signal.
+struct SemaphorPipe {
+  // Expecting two file-descriptors opened as a pipe in the canonical POSIX
----------------
Semaphor*e*


================
Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:42
+struct SemaphorPipe {
+  // Expecting two file-descriptors opened as a pipe in the canonical POSIX
+  // order: pipefd[0] refers to the read end of the pipe. pipefd[1] refers to
----------------
"Expects"

Three slashes for doc comments.


================
Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:49
+    assert(Result != -1);
+  };
+  ~SemaphorPipe() {
----------------
Extra semicolon.


================
Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:52
+    close(FDWrite);
+    close(FDRead);
+  }
----------------
Since it closes the file descriptors in the destructor, I feel like it should also be responsible for calling `pipe` in the constructor.


================
Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:58
+
+// Mutex-protected queue of Events.
+class EventQueue {
----------------
Three slashes for doc comments.


================
Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:120
+
+  // Consuming inotify events and pushing events to the Queue.
+  void InotifyPollingLoop();
----------------
"consumes", "pushes"


================
Comment at: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:109
+
+  // This method is used by DirectoryWatcher
+  void consume(DirectoryWatcher::Event E, bool IsInitial) {
----------------
Please add a period at the end of the comment (everywhere in the patch).


================
Comment at: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:153
+  // Not locking - caller has to lock Mtx
+  llvm::Optional<bool> Result() const {
+    if (ExpectedInitial.empty() && ExpectedNonInitial.empty() &&
----------------
Please name functions consistently -- there's both `consume()` that starts with lowercase, and `Result()` that starts with uppercase.

Please refer to the current naming rules in the style guide and apply everywhere in the patch.




================
Comment at: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:210
+
+  // TestConsumer didn't reach the expected state in given time.
+  EXPECT_TRUE(WaitForExpectedStateResult.wait_for(std::chrono::seconds(3)) ==
----------------
"If the following assertions fail, it is a sign that ..."

Also you can stream the message into the EXPECT_TRUE, it will be printed if the assertion fails.

EXPECT_TRUE(...) << "whatever";


================
Comment at: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:224
+
+TEST(DirectoryWatcherTest, initialScanSync) {
+  DirectoryWatcherTestFixture fixture;
----------------
Test names start with an uppercase letter (`InitialScanSync`).  Please apply everywhere in the patch.


================
Comment at: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:243
+      },
+      true);
+
----------------
Add /*waitForInitialSync=*/ ?  (everywhere in the patch)


================
Comment at: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:304
+      {
+          {DirectoryWatcher::Event::EventKind::Modified, "a"},
+      }};
----------------
Delete the comma and wrap onto one line.


================
Comment at: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:419
+
+    std::error_code setTimeRes = llvm::sys::fs::setLastAccessAndModificationTime(FD, NewTimePt,
+                                                            NewTimePt);
----------------
80 columns.


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

https://reviews.llvm.org/D58418