[PATCH] D105014: added some example code for llvm::Expected<T>

[Sam McCall via Phabricator via cfe-commits]

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