<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Wed, Dec 14, 2016 at 2:11 PM Greg Clayton <<a href="mailto:clayborg@gmail.com">clayborg@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br class="gmail_msg">
> On Dec 14, 2016, at 1:55 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="gmail_msg" target="_blank">dblaikie@gmail.com</a>> wrote:<br class="gmail_msg">
><br class="gmail_msg">
><br class="gmail_msg">
><br class="gmail_msg">
> On Wed, Dec 14, 2016 at 1:51 PM Greg Clayton via Phabricator <<a href="mailto:reviews@reviews.llvm.org" class="gmail_msg" target="_blank">reviews@reviews.llvm.org</a>> wrote:<br class="gmail_msg">
> clayborg created this revision.<br class="gmail_msg">
> clayborg added reviewers: aprantl, dblaikie, echristo, probinson, llvm-commits.<br class="gmail_msg">
> Herald added subscribers: jgosnell, mehdi_amini.<br class="gmail_msg">
><br class="gmail_msg">
> 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:<br class="gmail_msg">
><br class="gmail_msg">
>   uint64_t CallColumn = Die.getAttributeValueAsAddress(DW_AT_call_line, 0);<br class="gmail_msg">
><br class="gmail_msg">
> Has to be turned into:<br class="gmail_msg">
><br class="gmail_msg">
>   uint64_t CallColumn = 0;<br class="gmail_msg">
>   if (auto CallColumnValue = Die.getAttributeValueAsAddress(DW_AT_call_line))<br class="gmail_msg">
>       CallColumn = *CallColumnValue;<br class="gmail_msg">
><br class="gmail_msg">
> We could have a utility for this - either in Optional, or as a free function, so could write it as something like this:<br class="gmail_msg">
><br class="gmail_msg">
> auto CallColumn = Die.getAttributeValueAsAddress(DW_AT_call_line).withDefault(0);<br class="gmail_msg">
><br class="gmail_msg">
> for example.<br class="gmail_msg">
><br class="gmail_msg">
> (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:<br class="gmail_msg">
><br class="gmail_msg">
> = withDefault(..., 0);<br class="gmail_msg">
><br class="gmail_msg">
> )<br class="gmail_msg">
><br class="gmail_msg">
> Don't have to - just a thought.<br class="gmail_msg">
<br class="gmail_msg">
I like that idea. Add something like this to Optional.h?:<br class="gmail_msg">
<br class="gmail_msg">
template <typename T> T withDefault(const Optional<T> &Opt, T DefaultValue) {<br class="gmail_msg">
  if (Opt)<br class="gmail_msg">
    return *Opt;<br class="gmail_msg">
  return DefaultValue;<br class="gmail_msg">
}<br class="gmail_msg">
<br class="gmail_msg">
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?</blockquote><div><br>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:<br><br>const int &x = withDefault(opt, 3);<br><br> now 'x' is a dangling reference to 3.<br><br>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.</div></div></div>