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

[Dmitri Gribenko via Phabricator via cfe-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
----------------
jkorous wrote:
> plotfi wrote:
> > 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? 
> We shouldn't assert. Just return an error and let client code handle it in any way it wants.
I'm saying that it is not appropriate to document the API as `assert()`ing about something, because it is not a part of the contract.

You can say that the API *requires* the Path to be non-empty. Or you can call llvm_fatal_error explicitly if it is empty, and then you can document it. Or you can return an error like jkorous said. However, assertions are not a part of the API 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()");
 
----------------
jkorous wrote:
> plotfi wrote:
> > 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. 
> I think the suggestion is just
> 
> ```
> assert(!Path.empty());
> ```
Right. The standard assert already prints the expression -- in fact, this is how the `&& "xyz"` part gets printed when assertion fails -- assert prints the expression that failed.


================
Comment at: cfe/trunk/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:287
       /*waitForInitialSync=*/true);
+  if (!DW) return;
 
----------------
plotfi wrote:
> 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. 
I think I know how llvm::Expected works.

The problem that this check and return hides is that if DirectoryWatcher::create returns an error, the rest of the test is silently slipped. The test should be using something like `cantFail` instead.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65704