sammccall added a comment.
Yes, please revert the unrelated formatting changes.
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!)
However I don't think the "hump" you experienced can be avoided purely by
"recipe-style" documentation, because you can quickly get into a situation
where you have to understand Error's checking model to write proper handling
code.
By making the examples a bit more complete and adding a little more explanation
we can flatten it though!
================
Comment at: llvm/include/llvm/Support/Error.h:443
+/// // return an Error
+/// return error("B must not be zero!");
+/// }
----------------
the error() function is a clangd-ism (and was even controversial there!)
The "plain" equivalent is, sadly, `return
createStringError(inconvertibleErrorCode(), "B must not be zero!");`
================
Comment at: llvm/include/llvm/Support/Error.h:454
+/// if (!Result) {
+/// auto Error = Result.takeError();
+/// // handle the error case here
----------------
nit: we shouldn't call a variable Error if the type Error is in scope, it's
asking for trouble (and occasionally invalid code that some compilers reject
and others accept...)
================
Comment at: llvm/include/llvm/Support/Error.h:455
+/// auto Error = Result.takeError();
+/// // handle the error case here
+/// } else {
----------------
IMO the hard part here is that *the error must be consumed*/checked.
takeError doesn't actually do this, after `Err = Result.takeError()`, `Result`
is empty with Checked=true, but `Err` is populated and has Checked=false.
Confusingly, most of the things you can actually *do* with an Error (including
toString()) will check/consume it. But not all, and in particular if you do
nothing with it then `~Error()` will crash.
---
Assigning the Error to a separate variable is fairly rare and runs into more of
these complications, usually you rather either log or propagate it, in either
case using the rvalue directly.
I'd probably replace it with this code + comment:
```
// We must consume the error. Typically one of:
// - return the error to our caller
// - toString(), when logging
// - consumeError(), to silently swallow the error
// - handleErrors(), to distinguish error types
errs() << "Problem with division " << toString(Result.takeError()) << "\n";
```
================
Comment at: llvm/include/llvm/Support/Error.h:456
+/// // handle the error case here
+/// } else {
+/// // handle good case here
----------------
generally the "then" block would either propagate the error or otherwise
return, so you probably don't want else here.
================
Comment at: llvm/include/llvm/Support/Error.h:457
+/// } else {
+/// // handle good case here
+/// }
----------------
you haven't illustrated how to get the result.
Maybe
```
outs() << "The answer is " << *Result << "\n";
```
================
Comment at: llvm/include/llvm/Support/Error.h:462
+///
+/// Unit-testing a function returning an 'Expceted<T>':
+/// @code{.cpp}
----------------
This seems too intrusive/unusual to put inline to me.
And the main thing that is non-obvious is *why* you have to structure your
assertions in an unusual way, and that's not explained here.
Consider instead just briefly referencing the EXPECT_THAT_EXPECTED etc macros
in llvm/Testing/Support/Error.h which are designed to simplify this a bit.
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
