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

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi marked an inline comment 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:
> 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.
> 
> 
That makes sense. Lemme think about which is best, and I'll amend this diff.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65704