sammccall added a comment. In D105014#2872674 <https://reviews.llvm.org/D105014#2872674>, @dblaikie wrote:
> +@lhames for context as the author/owner of `llvm::Error` and associated > things. > > Perhaps it'd be handy to have some descriptions of the problems you > encountered, @kuhnel, and how you went about trying to resolve them, to help > understand where things are lacking. > >> @sammccall >> I agree Error is confusing and better docs might help. >> (Not as much as removing the confusing part of the design, but I that seems >> hard at this point!) > > Maybe in this review, or maybe elsewhere: might be good to have some details > on your perspective here? Mostly that the attempt to enforce handling is an interesting idea but a cure worse than the disease in practice. In interacting closely with ~6 developers over the last ~4 years: - most people were initially confused by this class and took significant time to build up a mental model, and are still fuzzy (e.g. on how it interacts with move semantics). I think this was Christian's experience here. - In practice, developers who know Error well still sometimes check in code with "unhandled" errors by mistake - most such code is correct except for the enforcement itself. e.g. test code that's missing a `cantFail(...)`, production code missing a `consumeError(Exp.takeError())` after handling failure. - In practice, these mistake are difficult for reviewers who know Error well to catch - it causes the Error class to have interface warts (`toString()` consumes the move-only object, operator== is non-const) that cause surprising ergonomic problems, e.g. requiring special helpers when used from test code We've talked about the possibility of adding separate Error/Expected types to clangd with similar semantics but no check-enforcement and a simpler payload model. However having to deal with both error types seems too painful. There's one case that's problematic enough to cause real danger, and has bitten us a couple of times: - an `Expected<T>` is obtained and checked for success - dynamically the value is almost always OK (e.g. except when out of disk space) - in the failure case, the error is handled but not consumed. This is not caught in tests because the error condition is hard to simulated - now months later when the error occurs in the wild in a debug build (yes, people seem to use these...) it becomes a crash --- Separately from this issue, creating/logging errors is pretty clunky in the common case, and error handling is ubiquitous. I wonder if there's appetite for the `error()` function from `clang-tools-extra/clangd/support/Logger.h` to live somewhere common. (Both `error()` and the support for errors in `log()` in that file are pretty useful quality-of-life features, but `log()` is maybe not well-suited for llvm) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105014/new/ https://reviews.llvm.org/D105014 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
