<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Mon, Oct 10, 2016 at 1:24 PM Duncan P. N. Exon Smith via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">> On 2016-Oct-10, at 12:35, Jordan Rose <<a href="mailto:jordan_rose@apple.com" class="gmail_msg" target="_blank">jordan_rose@apple.com</a>> wrote:<br class="gmail_msg">
><br class="gmail_msg">
> jordan_rose created this revision.<br class="gmail_msg">
> jordan_rose added reviewers: chandlerc, dexonsmith.<br class="gmail_msg">
> jordan_rose added a subscriber: llvm-commits.<br class="gmail_msg">
> jordan_rose set the repository for this revision to rL LLVM.<br class="gmail_msg">
><br class="gmail_msg">
> Without this, the following statements will create ArrayRefs that refer to temporary storage that goes out of scope by the end of the line:<br class="gmail_msg">
><br class="gmail_msg">
>  c++<br class="gmail_msg">
>  someArrayRef = getSingleElement();<br class="gmail_msg">
>  someArrayRef = {elem1, elem2};<br class="gmail_msg">
><br class="gmail_msg">
> Note that the constructor still has this problem:<br class="gmail_msg">
><br class="gmail_msg">
>  c++<br class="gmail_msg">
>  ArrayRef<Element> someArrayRef = getSingleElement();<br class="gmail_msg">
>  ArrayRef<Element> someArrayRef = {elem1, elem2};<br class="gmail_msg">
<br class="gmail_msg">
Long-term, I think these constructors should be explicit.<br class="gmail_msg">
<br class="gmail_msg">
> but that's a little harder to get rid of because we want to be able to use this in calls:<br class="gmail_msg">
><br class="gmail_msg">
>  c++<br class="gmail_msg">
>  takesArrayRef(getSingleElement());<br class="gmail_msg">
>  takesArrayRef({elem1, elem2});<br class="gmail_msg">
<br class="gmail_msg">
These would have to change to:<br class="gmail_msg">
<br class="gmail_msg">
    takesArrayRef(makeArrayRef(getSingleElement()));<br class="gmail_msg">
    takesArrayRef(makeArrayRef({elem1, elem2}));<br class="gmail_msg">
<br class="gmail_msg">
but I think that would be a fine tradeoff.<br class="gmail_msg"></blockquote><div><br>I wouldn't be so sure - we rely on these implicit conversions (similarly for StringRef) a /lot/. But maybe it's less bad than I imagine.<br><br>(what I'd really like is a clang-tidy check integrated into the compilation/build pipeline somehow... - I'm still waiting for that integration, at which point we can have project specific and otherwise "low value" (things that'd usually have too high signal/noise to add to Clang let alone enable by default in Clang) warnings/errors)<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br class="gmail_msg">
><br class="gmail_msg">
> Part of rdar://problem/16375365.<br class="gmail_msg">
><br class="gmail_msg">
><br class="gmail_msg">
> Repository:<br class="gmail_msg">
>  rL LLVM<br class="gmail_msg">
><br class="gmail_msg">
> <a href="https://reviews.llvm.org/D25446" rel="noreferrer" class="gmail_msg" target="_blank">https://reviews.llvm.org/D25446</a><br class="gmail_msg">
><br class="gmail_msg">
> Files:<br class="gmail_msg">
>  include/llvm/ADT/ArrayRef.h<br class="gmail_msg">
><br class="gmail_msg">
><br class="gmail_msg">
> Index: include/llvm/ADT/ArrayRef.h<br class="gmail_msg">
> ===================================================================<br class="gmail_msg">
> --- include/llvm/ADT/ArrayRef.h<br class="gmail_msg">
> +++ include/llvm/ADT/ArrayRef.h<br class="gmail_msg">
> @@ -219,6 +219,22 @@<br class="gmail_msg">
>       return Data[Index];<br class="gmail_msg">
>     }<br class="gmail_msg">
><br class="gmail_msg">
> +    /// Disallow accidental assignment from a temporary.<br class="gmail_msg">
> +    ///<br class="gmail_msg">
> +    /// The declaration here is extra complicated so that "arrayRef = {}"<br class="gmail_msg">
> +    /// continues to select the move assignment operator.<br class="gmail_msg">
> +    template <typename U><br class="gmail_msg">
> +    typename std::enable_if<std::is_same<U, T>::value, ArrayRef<T>>::type &<br class="gmail_msg">
> +    operator=(U &&Temporary) = delete;<br class="gmail_msg">
<br class="gmail_msg">
I'm not sure the parameter name, "Temporary", is worth spelling out.<br class="gmail_msg">
<br class="gmail_msg">
LGTM (with or without the name) if you add std::is_assignable-based static_asserts to unittests/ADT/ArrayRefTest.cpp.<br class="gmail_msg">
<br class="gmail_msg">
> +<br class="gmail_msg">
> +    /// Disallow accidental assignment from a temporary.<br class="gmail_msg">
> +    ///<br class="gmail_msg">
> +    /// The declaration here is extra complicated so that "arrayRef = {}"<br class="gmail_msg">
> +    /// continues to select the move assignment operator.<br class="gmail_msg">
> +    template <typename U><br class="gmail_msg">
> +    typename std::enable_if<std::is_same<U, T>::value, ArrayRef<T>>::type &<br class="gmail_msg">
> +    operator=(std::initializer_list<U>) = delete;<br class="gmail_msg">
> +<br class="gmail_msg">
>     /// @}<br class="gmail_msg">
>     /// @name Expensive Operations<br class="gmail_msg">
>     /// @{<br class="gmail_msg">
><br class="gmail_msg">
><br class="gmail_msg">
> <D25446.74161.patch><br class="gmail_msg">
<br class="gmail_msg">
_______________________________________________<br class="gmail_msg">
llvm-commits mailing list<br class="gmail_msg">
<a href="mailto:llvm-commits@lists.llvm.org" class="gmail_msg" target="_blank">llvm-commits@lists.llvm.org</a><br class="gmail_msg">
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" class="gmail_msg" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br class="gmail_msg">
</blockquote></div></div>