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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
