[LLVMdev] Addressing const reference in ArrayRef

[David Blaikie]

On Fri, Aug 22, 2014 at 10:16 AM, Duncan P. N. Exon Smith
<dexonsmith at apple.com> wrote:
>> On 2014-Aug-21, at 10:39, David Blaikie <dblaikie at gmail.com> wrote:
>>
>> On Thu, Aug 21, 2014 at 10:34 AM, Reid Kleckner <rnk at google.com> wrote:
>>> Is there some way we can get lifetime extension of temporaries to kick in
>>> here?
>>
>> Nope - since the temporary is a subexpression - not the thing being declared.
>>
>
>
>>>>>>> We could move this ctor into a factory function (and/or just make the
>>>>>>> CTor
>>>>>>> private and friend the makeArrayRef helper for this case) to make it
>>>>>>> more
>>>>>>> obvious/easier to find bad call sites. But it would be helpful to
>>>>>>> have
>>>>>>> the
>>>>>>> context of the prior discussion to continue from there.
>
> A variation of these solutions makes sense to me.  I strongly dislike
> this constructor since it leads to other subtle bugs.  E.g., suppose you
> have this API:
>
>     void foo(ArrayRef<const char *> Strings);
>
> and some caller uses `foo("string")`.
>
> If you change the API to add an optional `bool`:
>
>     void foo(ArrayRef<const char *> Strings, bool B = false);
>     void foo(bool B = false);
>
> the caller silently switches to `foo(bool("string"))` instead of
> `foo(ArrayRef<const char *>("string"))`.

My perspective on this /\ is that this kind of API refactoring problem
is just one instance of a broader problem (replace ArrayRef with int,
T*, etc in the above and you're back to square one). We could catch
some instances (such as the one you've shown) with a broader
-Wliteral-conversion warning (any literal other than "true" or "false"
should warn if converted to bool - modulo the usual false positive
suppressions... (RHS of an && for assertions, etc)). Of course that
would be thwarted by moving the string into a local variable. Nothing
perfect here.

> If we remove the `ArrayRef` constructor AND the helper, and add a new
> helper like:
>
>     template <class T> ArrayRef<T> makeTempArrayRef(const T &OneElt) {
>       return ArrayRef<T>(&OneElt, &OneElt + 1);
>     }
>
> then both types of bugs will be more rare and obvious.
>
> For the OP's case, the name "Temp" makes it more clear that the ArrayRef
> shouldn't be named.  For my case, since the caller is forced to use
> `foo(makeTempArrayRef("string"))`, it's easier to make safe API changes.

Yep - the convenience of one-element->ArrayRef is "cute" at best, I
think. Having to wrap it doesn't seem detrimental. Would have to look
at some numbers, though (if we could easily gather the number of
instances of this - I'm not sure of the easiest way to do that, given
the build system likes to stop after it sees an error).

I'm not sure if the "Temp" in the name would be a sufficient
deterrent, but maybe.

> This would cause a lot of churn though -- ArrayRefs are *usually*
> temporaries.  Although this makes sense to me, disallowing named
> ArrayRefs might be more practical.

Yeah, I'm not sure how practical that is either - we have (guessing)
quite a few named ArrayRefs... but it might be tractible to remove
them. Not sure how I feel about that.