plotfi marked 6 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 ---------------- 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? ================ 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()"); ---------------- 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. ================ Comment at: cfe/trunk/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:287 /*waitForInitialSync=*/true); + if (!DW) return; ---------------- 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. 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