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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
