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