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

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

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