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.
Duncan P. N. Exon Smith
2014-Aug-22 21:34 UTC
[LLVMdev] Addressing const reference in ArrayRef
> On 2014-Aug-22, at 11:37, David Blaikie <dblaikie at gmail.com> wrote: > > 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 >> 6083 > > Uniqued? I imagine a few are in headers.Good point. $ grep Wdeprecated-declarations warnings.log | sort -u | wc -l 644
On Fri, Aug 22, 2014 at 2:34 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:> >> On 2014-Aug-22, at 11:37, David Blaikie <dblaikie at gmail.com> wrote: >> >> 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 >>> 6083 >> >> Uniqued? I imagine a few are in headers. > > Good point. > > $ grep Wdeprecated-declarations warnings.log | sort -u | wc -l > 644Thanks! I'll try to sum up my thoughts on this: That seems like many enough that I'd rather not force all those callers to use an explicit local array. The utility function doesn't actually save us - we still have to be diligent about not using it for named ArrayRefs. But it might be enough... A clang-tidy check would be great (instead of any API change) - if we had a decent developer integration, clang-tidy must-clean shortlist that was on a buildbot, etc. All that being said - I don't have any great alternative suggestions, nor do I feel personally compelled to do any of the work listed above, so if anyone feels like doing any of those things (even the ones I don't much like) I don't think I'll stand in the way of progress :) - David