dexonsmith added inline comments.
================
Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:154
+ errorMsg += llvm::sys::StrError();
+ return true;
+ };
----------------
jkorous wrote:
> mgorny wrote:
> > The return values seem to be confusing. Any reason not to use `true` for
> > success?
> It seems there's no real consensus * about error/success mapping to bool and
> since this is just implementation detail (not a public interface) I think
> it's fine. `llvm::Error` has better semantics but seems a bit heavyweight for
> this.
>
> * Just tried this: grep -nr "returns true" clang | grep -i error
Agreed that much of LLVM/Clang uses the C convention where `false`/`0` is
success.
However, I'm curious whether it might be better to thread `llvm::Error` (and/or
`llvm::Expected`) through, since you bring it up. They provide nice assertions
when the caller forgets to check the result, they're cheap on success, and
there's no true/false ambiguity.
================
Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:94
+ std::shared_ptr<EventQueue> evtQueue) {
+#define EVT_BUF_LEN (30 * (sizeof(struct inotify_event) + NAME_MAX + 1))
+ char buf[EVT_BUF_LEN] __attribute__((aligned(8)));
----------------
Can this be `constexpr int EventBufferLength` instead of `#define EVT_BUF_LEN`?
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
