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