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

Reply via email to