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.
On Fri, Aug 22, 2014 at 10:43 AM, David Blaikie <dblaikie at gmail.com> wrote:> > 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.Wow, I think I finally understand how the temporary is being created. Can we get by without the single element constructor? Callers would have to write: const char *strs[] = { "string" }; foo(strs); Instead of foo("string"), which is subtle anyway. -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140822/98a94614/attachment.html>
On Fri, Aug 22, 2014 at 10:50 AM, Reid Kleckner <rnk at google.com> wrote:> On Fri, Aug 22, 2014 at 10:43 AM, David Blaikie <dblaikie at gmail.com> wrote: >> >> 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. > > > Wow, I think I finally understand how the temporary is being created.Subtle, innit?> > Can we get by without the single element constructor? Callers would have to > write: > const char *strs[] = { "string" }; > foo(strs);We can get by - I think that'd be a tad verbose though. But we don't quite have a feel for just how many of these single-element-conversions there are.> Instead of foo("string"), which is subtle anyway.Yeah - I think it's "cute" at best, and wouldn't mind something more explicit - I'm just not sure defining a whole local array is the right tradeoff either.
Duncan P. N. Exon Smith
2014-Aug-22 18:22 UTC
[LLVMdev] Addressing const reference in ArrayRef
> On 2014-Aug-22, at 10:43, David Blaikie <dblaikie at gmail.com> wrote: > > 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).diff --git a/include/llvm/ADT/ArrayRef.h b/include/llvm/ADT/ArrayRef.h index 0351cf5..79f14da 100644 --- a/include/llvm/ADT/ArrayRef.h +++ b/include/llvm/ADT/ArrayRef.h @@ -68,7 +68,7 @@ namespace llvm { /*implicit*/ ArrayRef(NoneType) : Data(nullptr), Length(0) {} /// Construct an ArrayRef from a single element. - /*implicit*/ ArrayRef(const T &OneElt) + __attribute__((deprecated)) /*implicit*/ ArrayRef(const T &OneElt) : Data(&OneElt), Length(1) {} /// Construct an ArrayRef from a pointer and length. and rebuilding LLVM and Clang gives me: $ ninja 2>&1 | grep -e Wdeprecated-declarations | wc -l 6083 This includes forwards from the `makeArrayRef()` helper.
On Fri, Aug 22, 2014 at 11:22 AM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:> >> On 2014-Aug-22, at 10:43, David Blaikie <dblaikie at gmail.com> wrote: >> >> 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). > > diff --git a/include/llvm/ADT/ArrayRef.h b/include/llvm/ADT/ArrayRef.h > index 0351cf5..79f14da 100644 > --- a/include/llvm/ADT/ArrayRef.h > +++ b/include/llvm/ADT/ArrayRef.h > @@ -68,7 +68,7 @@ namespace llvm { > /*implicit*/ ArrayRef(NoneType) : Data(nullptr), Length(0) {} > > /// Construct an ArrayRef from a single element. > - /*implicit*/ ArrayRef(const T &OneElt) > + __attribute__((deprecated)) /*implicit*/ ArrayRef(const T &OneElt) > : Data(&OneElt), Length(1) {} > > /// Construct an ArrayRef from a pointer and length. > > and rebuilding LLVM and Clang gives me: > > $ ninja 2>&1 | grep -e Wdeprecated-declarations | wc -l > 6083Uniqued? I imagine a few are in headers.> > This includes forwards from the `makeArrayRef()` helper.