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

[Puyan Lotfi via Phabricator via llvm-commits]

plotfi marked 6 inline comments as done.
plotfi 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
----------------
gribozavr wrote:
> 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.
Please be more specific. What logic are you talking about? 


================
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()");
 
----------------
gribozavr wrote:
> 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.
This is std assert. Are you referring to a different assert? If @compnerd is ok with changing the assert into an llvm::Expected I can do that. 


================
Comment at: cfe/trunk/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:287
       /*waitForInitialSync=*/true);
+  if (!DW) return;
 
----------------
gribozavr wrote:
> Why? This is a test -- ignoring errors would only hide them from developers.
Please see how llvm::Expected works. This does not ignore or hide anything. 


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65704