plotfi marked an inline comment 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: > 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. 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