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

Thu Oct 21 18:10:16 PDT 2021

dblaikie 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)
----------------
dexonsmith wrote:
> 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.
Fair ponit re: the other operations clearly don't consume the /whole/ object, you have to use both operations. Whereas this new API does consume the whole object, there's nothing else you can do with it after using this operation, I think? (since the value and the error have been moved-from)

So, yeah, at that point I do lean in favor of `&&`. Not strongly, but yeah, probably enough to encourage it, give it a go, we can remove it later as you say.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112278