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