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

[Jan Korous via Phabricator via cfe-commits]

jkorous marked 20 inline comments as done.
jkorous added inline comments.


================
Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:40
+// FDRead.
+// Currently used just for one-off termination signal.
+struct SemaphorPipe {
----------------
gribozavr wrote:
> 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.)
Sure, no problem.

I naturally tend to err on the side of over-commenting as I think I'd appreciate it myself if I had to understand the code without prior knowledge - not saying it's intentional or that I have a strong reason to do that though. You seem to have a strong preference for not having out-of-date comments with low-ish information value. Just out of curiosity - is it based on any particular reason or experience?


================
Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:52
+    close(FDWrite);
+    close(FDRead);
+  }
----------------
gribozavr wrote:
> Since it closes the file descriptors in the destructor, I feel like it should also be responsible for calling `pipe` in the constructor.
I know what you mean - my "oop feel" was telling me it's wrong too. It's not just the pipe in this class but also the inotify descriptor in the watcher class.

The problem is that the only reasonable way how to communicate failures from constructors that I am aware of are exceptions which we don't use in llvm. That's why I moved most of the stuff that can fail even before any work is actually started to the factory method (with the exception of epoll file descriptor as that felt like making its scope unnecessarily larger).

Thinking about it now, I am starting to doubt that it makes life any easier for client code as it still has to cope with failure communicated as WatcherGotInvalidated event via receiver.

What do you think?


================
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() &&
----------------
gribozavr wrote:
> 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.
> 
> 
Sorry about that. This is definitely my weak point.


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

https://reviews.llvm.org/D58418