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