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

[Jan Korous via Phabricator via cfe-commits]

jkorous marked 4 inline comments as done.
jkorous added inline comments.


================
Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:116
+
+      DirectoryWatcher::EventKind K = DirectoryWatcher::EventKind::Added;
+      if (ievt->mask & IN_MODIFY) {
----------------
mgorny wrote:
> I don't think I understand this assumption. Have you any specific event in mind that is supposed to be treated as added here? If not, maybe it'd be better to have no default and instead assert that one of the conditions below matched.
SGTM. Will rewrite to Optional<> and assert.


================
Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:135
+        if (!statusOpt.hasValue())
+          K = DirectoryWatcher::EventKind::Removed;
+      }
----------------
mgorny wrote:
> Why? I suppose this deserves a comment.
I'll add this comment:

// The file might have been removed just after we received the event.


================
Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:154
+    errorMsg += llvm::sys::StrError();
+    return true;
+  };
----------------
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


================
Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher.cpp:95
+
+#if !defined(__has_include)
+#define __has_include(x) 0
----------------
mgorny wrote:
> Why not use CMake checks? You're already partially using them for frameworks.
Do I understand correctly that you suggest doing the check in CMake and propagating the result to preprocessor?


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

https://reviews.llvm.org/D58418