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