[PATCH] D27772: Add the ability to get attribute values as Optional<T>

[David Blaikie via llvm-commits]

On Wed, Dec 14, 2016 at 2:11 PM Greg Clayton <clayborg at gmail.com> wrote:

>
> > On Dec 14, 2016, at 1:55 PM, David Blaikie <dblaikie at gmail.com> wrote:
> >
> >
> >
> > On Wed, Dec 14, 2016 at 1:51 PM Greg Clayton via Phabricator <
> reviews at reviews.llvm.org> wrote:
> > clayborg created this revision.
> > clayborg added reviewers: aprantl, dblaikie, echristo, probinson,
> llvm-commits.
> > Herald added subscribers: jgosnell, mehdi_amini.
> >
> > When getting attributes it is sometimes nicer to use Optional<T> some of
> the time instead of magic values. I tried to cut over to only using the
> Optional values but it made many of the call sites very messy, so it makes
> sense the leave in the calls that can return a default value. Otherwise
> code that looks like this:
> >
> >   uint64_t CallColumn = Die.getAttributeValueAsAddress(DW_AT_call_line,
> 0);
> >
> > Has to be turned into:
> >
> >   uint64_t CallColumn = 0;
> >   if (auto CallColumnValue =
> Die.getAttributeValueAsAddress(DW_AT_call_line))
> >       CallColumn = *CallColumnValue;
> >
> > We could have a utility for this - either in Optional, or as a free
> function, so could write it as something like this:
> >
> > auto CallColumn =
> Die.getAttributeValueAsAddress(DW_AT_call_line).withDefault(0);
> >
> > for example.
> >
> > (actually, since we'll probably want to migrate from llvm::Optional to
> std::optional one day (when we get it, etc) - probably best to keep it as a
> non-member:
> >
> > = withDefault(..., 0);
> >
> > )
> >
> > Don't have to - just a thought.
>
> I like that idea. Add something like this to Optional.h?:
>
> template <typename T> T withDefault(const Optional<T> &Opt, T
> DefaultValue) {
>   if (Opt)
>     return *Opt;
>   return DefaultValue;
> }
>
> This would be useful for simple type "T" values. Not sure if we should
> return a "T&" and have "const T& DefaultValue" as the second parameter?


That's the trick with this sort of API, unfortunately. If it takes a
reference and returns one, you can accidentally end up with dangling
references:

const int &x = withDefault(opt, 3);

 now 'x' is a dangling reference to 3.

But, as Adrian said - this change will probably go separately, with a unit
test, and we'll get some discussion from people with an interest in the
general ADT design & these sort of nuances.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161214/b85d94b6/attachment.html>