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

Mon Jun 3 17:57:00 PDT 2019

jkorous marked 3 inline comments as done.
jkorous added a comment.

I fixed the rest.

There are still some questions you raised that I just responded to and kept them as not Done. Feel free to take a look. If nothing comes up I'll commit this on Wednesday.

================
Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:40
+// FDRead.
+// Currently used just for one-off termination signal.
+struct SemaphorPipe {
----------------
gribozavr wrote:
> jkorous wrote:
> > 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?
> > is it based on any particular reason or experience?
> 
> Yes, primarily working on the Swift compiler and the standard library, when everything was changing very quickly.  My current work also confirms the same -- a lot of time when I see a comment about the "current usage" or other incidental information that is not the contract of the API, it tends to be outdated.  Approximately nobody will change such a comment when another user is added ("I'm just reusing the code..."), even when the quoted user already became non-representative of the usage pattern, or the usage pattern has changed.  However, when changing the API contract people typically do change the comment.
> 
> It also makes sense to me in abstract: reading that X happens to be used for Y does not necessarily help understand X better -- it is only a cross-reference that I could find myself with an IDE command; I still need to understand the design of Y and the interaction with X, and then using my past experience infer what X was intended to be.
> 
> Saying that X is intended to be used only by Y is a different story of course, that's design documentation.
> 
> Providing an example of usage is also fine, but it should be phrased as an example that can't become stale.
Thank you.

================
Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:52
+    close(FDWrite);
+    close(FDRead);
+  }
----------------
gribozavr wrote:
> jkorous wrote:
> > 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?
> You could add a factory function to SemaphorePipe, but... I feel like trying to recover from a failure in the pipe() call is a bit like trying to recover from a memory allocation failure.  The process is probably hitting a file descriptor limit or something like that, and is likely going to fail anyway.  I'd probably trigger a fatal error if pipe() fails.
> 
> This class is a lot like pthread_mutex_init -- it can fail "gracefully", but there's no way for the caller to recover -- the caller needs a mutex to proceed.
I see what you mean and mostly agree. Anyways, let's allow clients to handle such funny moments themselves as much as they can.

I'll factor out the pipe call to the factory method.

================
Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:317
+
+  if (pipe(InotifyPollingStopperFDs) == -1)
+    return nullptr;
----------------
gribozavr wrote:
> Use pipe2() with O_CLOEXEC, to avoid leaking the file descriptors to child processes?
You're right, I didn't account for this. Added the flag also to `inotify_init1()` and `epoll_create1()` calls.

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

https://reviews.llvm.org/D58418