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

[Puyan Lotfi via Phabricator via llvm-commits]

plotfi marked 2 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
----------------
plotfi wrote:
> 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.
@jkorous I have another patch ready that will replace the asserts with llvm fatal_error. If this is good by you I will land that.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65704