[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

[Dmitri Gribenko via Phabricator via llvm-commits]

gribozavr added inline comments.


================
Comment at: cfe/trunk/include/clang/DirectoryWatcher/DirectoryWatcher.h:102
 
-  /// Returns nullptr if \param Path doesn't exist or isn't a directory.
-  /// Returns nullptr if OS kernel API told us we can't start watching. In such
-  /// case it's unclear whether just retrying has any chance to succeeed.
-  static std::unique_ptr<DirectoryWatcher>
+  /// Asserts if \param Path doesn't exist or isn't a directory.
+  /// Returns llvm::Expected Error if OS kernel API told us we can't start
----------------
I don't see where this logic is implemented.

Also, LLVM is often built with assertions disabled, so "asserts" can't be a part of the contract.


================
Comment at: cfe/trunk/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp:153
     dispatch_queue_t Queue) {
-  if (Path.empty())
-    return nullptr;
+  assert(!Path.empty() && "Path.empty()");
 
----------------
No need to repeat the condition in the `&& "..."` part. assert does that by default. Use an extra string to explain the assertion to the reader.


================
Comment at: cfe/trunk/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:287
       /*waitForInitialSync=*/true);
+  if (!DW) return;
 
----------------
Why? This is a test -- ignoring errors would only hide them from developers.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65704