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