[PATCH] D112278: Support: Add Expected<T>::moveInto() to avoid extra names

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith added inline comments.


================
Comment at: llvm/include/llvm/Support/Error.h:584
+      OtherT &Value,
+      std::enable_if_t<std::is_assignable<OtherT &, T &&>::value> * = nullptr) {
+    if (*this)
----------------
dblaikie wrote:
> Should this be && qualified, so it can only be called on a temporary Expected, since it's going to consume it? (I mean `takeError` isn't rvalue ref qualified and `get` isn't - so it's not clear it has to be)
> Should this be && qualified, so it can only be called on a temporary Expected, since it's going to consume it?

Maybe? I can't think of why you'd want to call this on a non-temporary. I don't see any problems with doing it though... and I suppose it'd guide people toward the pattern of not naming it?

Happy to do it; if you feel strongly, let me know; otherwise I'll stew for a bit. Leaning toward your suggestion, and then `&&` can be removed later if it turns out there's a good reason to allow it on lvalues.

> (I mean `takeError` isn't rvalue ref qualified and `get` isn't - so it's not clear it has to be)

I don't feel like `&&` is quite right for these APIs though. You need to name the value if you're going to both check the error and extract the value. Any `std::move()`s after that would feel like extra boilerplate.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112278/new/

https://reviews.llvm.org/D112278