[PATCH] D25446: Disallow ArrayRef assignment from temporaries

[David Blaikie via llvm-commits]

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.

(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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161017/e9c0ecd7/attachment.html>