gribozavr accepted this revision.
gribozavr added inline comments.
This revision is now accepted and ready to land.
================
Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:40
+// FDRead.
+// Currently used just for one-off termination signal.
+struct SemaphorPipe {
----------------
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.
================
Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:52
+ close(FDWrite);
+ close(FDRead);
+ }
----------------
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.
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
