[PATCH] D85072: [ADT] Add getAsOr to Optional

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D85072#2190302 <https://reviews.llvm.org/D85072#2190302>, @njames93 wrote:

> In D85072#2189300 <https://reviews.llvm.org/D85072#2189300>, @dblaikie wrote:
>
>> I forget: Have we already decided that Optional won't be replaced by std::optional? If not, then would be good to not add API surface area that doesn't match std::optional (instead preferring non-member functions, for instance).
>> If that's already been decided in some previous reviews, etc, this sounds OK I guess.
>
> If that's the case then I'd agree with that.
>  But I kind of gathered it wouldn't be the case considering how much the `llvm::Optional` API already differs from `std::optional`.

Hmm, I'm not so sure - getValueOr, for instance, is in std::optional. (value_or)

You're right that 'map', for instance, is not in std::optional, though. That and maybe the "create" function are the non-std::optional API?

I think if this new function was a non-member, it'd probably tidier - wouldn't need an rvalue and non-rvalue overload, as it could use std::forward on both arguments.



================
Comment at: llvm/include/llvm/ADT/Optional.h:295-297
+  // Delete this as it can lead to dangling reference issues if ``U`` references
+  // the value held in the Optional.
+  template <typename U> U getAsOr(U Default) && = delete;
----------------
I have reservations about this - for the same reason that StringRef, for instance, doesn't block being built from an rvalue (because you might (& in StringRef's case, do quite often) create one from a temporary only for use within a single statement).

Matching the sort of overload/semantics of getValueOr seems appropriate here.

Is there a particular example you have in mind where this would be a problem?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85072