[llvm] r283798 - Disallow ArrayRef assignment from temporaries.

[Jordan Rose via llvm-commits]

Fair enough. Thanks, Zachary. I'll probably end up just disabling the static-asserts on MSVC.

> On Oct 10, 2016, at 14:46, Zachary Turner <zturner at google.com> wrote:
> 
> Hi Jordan,
> 
> I have to revert this as it is causing build failures on MSVC 2015 with the following messages:
> 
>     ArrayRefTest.cpp(38): error C2338: Assigning from single prvalue element
>     ArrayRefTest.cpp(41): error C2338: Assigning from single xvalue element
>     ArrayRefTest.cpp(47): error C2338: Assigning from an initializer list
> 
> Unfortunately I don't have time to dig further at the moment and we're having bot failures pile up right now so I'm trying to get back to green.
> 
> On Mon, Oct 10, 2016 at 2:06 PM Jordan Rose via llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
> Author: jrose
> Date: Mon Oct 10 15:57:33 2016
> New Revision: 283798
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=283798&view=rev <http://llvm.org/viewvc/llvm-project?rev=283798&view=rev>
> Log:
> Disallow ArrayRef assignment from temporaries.
> 
> Without this, the following statements will create ArrayRefs that
> refer to temporary storage that goes out of scope by the end of the
> line:
> 
>   someArrayRef = getSingleElement();
>   someArrayRef = {elem1, elem2};
> 
> Note that the constructor still has this problem:
> 
>   ArrayRef<Element> someArrayRef = getSingleElement();
>   ArrayRef<Element> someArrayRef = {elem1, elem2};
> 
> but that's a little harder to get rid of because we want to be able to
> use this in calls:
> 
>   takesArrayRef(getSingleElement());
>   takesArrayRef({elem1, elem2});
> 
> Part of rdar://problem/16375365. Reviewed by Duncan Exon Smith.
> 
> Modified:
>     llvm/trunk/include/llvm/ADT/ArrayRef.h
>     llvm/trunk/unittests/ADT/ArrayRefTest.cpp
> 
> Modified: llvm/trunk/include/llvm/ADT/ArrayRef.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/ArrayRef.h?rev=283798&r1=283797&r2=283798&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/ArrayRef.h?rev=283798&r1=283797&r2=283798&view=diff>
> ==============================================================================
> --- llvm/trunk/include/llvm/ADT/ArrayRef.h (original)
> +++ llvm/trunk/include/llvm/ADT/ArrayRef.h Mon Oct 10 15:57:33 2016
> @@ -219,6 +219,22 @@ namespace llvm {
>        return Data[Index];
>      }
> 
> +    /// Disallow accidental assignment from a temporary.
> +    ///
> +    /// The declaration here is extra complicated so that "arrayRef = {}"
> +    /// continues to select the move assignment operator.
> +    template <typename U>
> +    typename std::enable_if<std::is_same<U, T>::value, ArrayRef<T>>::type &
> +    operator=(U &&Temporary) = delete;
> +
> +    /// Disallow accidental assignment from a temporary.
> +    ///
> +    /// The declaration here is extra complicated so that "arrayRef = {}"
> +    /// continues to select the move assignment operator.
> +    template <typename U>
> +    typename std::enable_if<std::is_same<U, T>::value, ArrayRef<T>>::type &
> +    operator=(std::initializer_list<U>) = delete;
> +
>      /// @}
>      /// @name Expensive Operations
>      /// @{
> 
> Modified: llvm/trunk/unittests/ADT/ArrayRefTest.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ADT/ArrayRefTest.cpp?rev=283798&r1=283797&r2=283798&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ADT/ArrayRefTest.cpp?rev=283798&r1=283797&r2=283798&view=diff>
> ==============================================================================
> --- llvm/trunk/unittests/ADT/ArrayRefTest.cpp (original)
> +++ llvm/trunk/unittests/ADT/ArrayRefTest.cpp Mon Oct 10 15:57:33 2016
> @@ -31,6 +31,21 @@ static_assert(
>      !std::is_convertible<ArrayRef<volatile int *>, ArrayRef<int *>>::value,
>      "Removing volatile");
> 
> +// Check that we can't accidentally assign a temporary location to an ArrayRef.
> +// (Unfortunately we can't make use of the same thing with constructors.)
> +static_assert(
> +    !std::is_assignable<ArrayRef<int *>, int *>::value,
> +    "Assigning from single prvalue element");
> +static_assert(
> +    !std::is_assignable<ArrayRef<int *>, int * &&>::value,
> +    "Assigning from single xvalue element");
> +static_assert(
> +    std::is_assignable<ArrayRef<int *>, int * &>::value,
> +    "Assigning from single lvalue element");
> +static_assert(
> +    !std::is_assignable<ArrayRef<int *>, std::initializer_list<int *>>::value,
> +    "Assigning from an initializer list");
> +
>  namespace {
> 
>  TEST(ArrayRefTest, AllocatorCopy) {
> @@ -161,6 +176,14 @@ TEST(ArrayRefTest, InitializerList) {
>    ArgTest12({1, 2});
>  }
> 
> +TEST(ArrayRefTest, EmptyInitializerList) {
> +  ArrayRef<int> A = {};
> +  EXPECT_TRUE(A.empty());
> +
> +  A = {};
> +  EXPECT_TRUE(A.empty());
> +}
> +
>  // Test that makeArrayRef works on ArrayRef (no-op)
>  TEST(ArrayRefTest, makeArrayRef) {
>    static const int A1[] = {1, 2, 3, 4, 5, 6, 7, 8};
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits <http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161010/b646038a/attachment.html>