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

[Jan Korous via Phabricator via cfe-commits]

jkorous marked 36 inline comments as done.
jkorous added a comment.

I addressed most of the comments.

What is left:

- rewrite test with gmock matchers
- write test for metadata of a file in watched dir being changed



================
Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:46
+    /// The watched directory got deleted. No more events will follow.
+    DirectoryDeleted,
+  };
----------------
gribozavr wrote:
> `DirectoryRemoved` (for consistency with `Removed`)
> 
> Also, how about `TopDirectoryRemoved`?  Otherwise it sounds like some random directory was removed.
I used `WatchedDirRemoved`.


================
Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:62
+                                                  bool waitInitialSync,
+                                                  std::string &Error);
+
----------------
gribozavr wrote:
> Why not return `llvm::ErrorOr<std::unique_ptr<DirectoryWatcher>>`?
> 
> I also don't understand why we need unique_ptr.  If you change `Implementation &Impl;` to `std::unique_ptr<Implementation> Impl;`, `DirectoryWatcher` becomes a move-only type, and `create` can return `llvm::ErrorOr<DirectoryWatcher>`.
I personally didn't like the C-like `std::string& error` in the interface. But I feel like having a polymorphic class (which doesn't really fit into `ErrorOr`) is lesser evil than the original pimpl design.
WDYT?


================
Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:95
+    EventKind Kind;
+    /// Filename - a relative path to the watched directory or empty string in
+    /// case event is related to the directory itself.
----------------
gribozavr wrote:
> gribozavr wrote:
> > gribozavr wrote:
> > > Don't repeat field names in comments.
> > "a relative path" -- relative to what?
> Is it really a path to the directory?
I reworded the comment.


================
Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:123
+                       DirectoryWatcher::EventReceiver Receiver,
+                       bool WaitInitialSync, std::string &Error);
+
----------------
gribozavr wrote:
> Make it a static function in the DirectoryWatcher?
You're right - seems like static factory methods are the LLVM way.


================
Comment at: clang/unittests/DirectoryWatcher/CMakeLists.txt:16
+if(APPLE)
+  check_include_files("CoreServices/CoreServices.h" HAVE_CORESERVICES_H)
+  if(HAVE_CORESERVICES_H)
----------------
gribozavr wrote:
> No need to check.  macOS will always have this file.  If it is not there, it is a big issue anyway.
Actually this is wrong but for a different reason - these are link dependencies of the implementation. Tests (or any other client) shouldn't care about it.


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

https://reviews.llvm.org/D58418