[PATCH] D25446: Disallow ArrayRef assignment from temporaries

[Duncan P. N. Exon Smith via llvm-commits]

> On 2016-Oct-17, at 09:44, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
> On Mon, Oct 10, 2016 at 1:24 PM Duncan P. N. Exon Smith via llvm-commits <llvm-commits at lists.llvm.org> wrote:
> > On 2016-Oct-10, at 12:35, Jordan Rose <jordan_rose at apple.com> wrote:
> >
> > jordan_rose created this revision.
> > jordan_rose added reviewers: chandlerc, dexonsmith.
> > jordan_rose added a subscriber: llvm-commits.
> > jordan_rose set the repository for this revision to rL LLVM.
> >
> > Without this, the following statements will create ArrayRefs that refer to temporary storage that goes out of scope by the end of the line:
> >
> >  c++
> >  someArrayRef = getSingleElement();
> >  someArrayRef = {elem1, elem2};
> >
> > Note that the constructor still has this problem:
> >
> >  c++
> >  ArrayRef<Element> someArrayRef = getSingleElement();
> >  ArrayRef<Element> someArrayRef = {elem1, elem2};
> 
> Long-term, I think these constructors should be explicit.
> 
> > but that's a little harder to get rid of because we want to be able to use this in calls:
> >
> >  c++
> >  takesArrayRef(getSingleElement());
> >  takesArrayRef({elem1, elem2});
> 
> These would have to change to:
> 
>     takesArrayRef(makeArrayRef(getSingleElement()));
>     takesArrayRef(makeArrayRef({elem1, elem2}));
> 
> but I think that would be a fine tradeoff.
> 
> 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.

I had a look a few years ago and abandoned; it would/will be a lot of pain to make the change.  But IMO the end result is better than the current state.

> (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)
>  
> 
> >
> > Part of rdar://problem/16375365.
> >
> >
> > Repository:
> >  rL LLVM
> >
> > https://reviews.llvm.org/D25446
> >
> > Files:
> >  include/llvm/ADT/ArrayRef.h
> >
> >
> > Index: include/llvm/ADT/ArrayRef.h
> > ===================================================================
> > --- include/llvm/ADT/ArrayRef.h
> > +++ include/llvm/ADT/ArrayRef.h
> > @@ -219,6 +219,22 @@
> >       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;
> 
> I'm not sure the parameter name, "Temporary", is worth spelling out.
> 
> LGTM (with or without the name) if you add std::is_assignable-based static_asserts to unittests/ADT/ArrayRefTest.cpp.
> 
> > +
> > +    /// 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
> >     /// @{
> >
> >
> > <D25446.74161.patch>
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits