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

[Jan Korous via Phabricator via cfe-commits]

jkorous 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
----------------
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.


================
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()");
 
----------------
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());
```


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65704