[llvm] r289579 - ADT: Add OwningArrayRef class.

Tue Dec 20 14:41:49 PST 2016

> On Dec 20, 2016, at 11:57 AM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> I tend to agree with John/Peter that a dynarray-like data structure is worth having around.  There are use cases where the memory overhead of std::vector is relevant; this strips it down a little.  It seems useful.
> 
> However, I'm skeptical of having a container inherit from ArrayRef (or MutableArrayRef).  Not all of the ArrayRef API really makes sense for a container.  And anything named ArrayRef is going to trigger people into thinking that operations are "cheap", when in point of fact, every operation here involves a malloc/delete[] pair.

Well, if by "every operation" you mean "just the operation of constructing it from an ArrayRef".

>  If we're going to have this, I think it should be called llvm::DynArray or something.

I would not object to this being a unrelated type as long as you can easily get a MutableArrayRef from it.

John.

> 
>> On 2016-Dec-19, at 23:12, David Blaikie via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>> 
>> Fair point-ish. Though my argument is a bit different since this is has a more direct analogy with something in the standard than some of the other ADTs we have that have more clear trade-offs against the things in the standard. In this case it's basically the exact thing that was considered and not standardized, as I understand it. So figure there might be some context there that could be relevant.
>> 
>> 
>> On Mon, Dec 19, 2016, 10:00 PM John McCall <rjmccall at apple.com> wrote:
>>> On Dec 19, 2016, at 9:47 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>> On Mon, Dec 19, 2016 at 9:24 PM Peter Collingbourne <peter at pcc.me.uk> wrote:
>>> Sure, I understood what you meant. I meant that I wouldn't take a position on whether avoiding the cost of the capacity and the reserve area is worth it.
>>> 
>>> (If pressed I think I'd say no, the average tu doesn't have that many vtables, and there are far more egregious wastes of memory in llvm anyway (e.g. llvm::DIE) that we should be concentrating on first, but while I was adding another thing to vtables I figured it wouldn't hurt to be consistent with the others, then rule of three kicked in so seemed reasonable to add the abstraction.)
>>> 
>>> Yeah, that's pretty much how I feel too.
>>> 
>>> Richard, Chandler - I seem to recall this has come up before (whether or not LLVM would benefit from a dynarray like abstraction) & I don't remember the backstory on the standards committee for dynarary (which I would've only heard second hand from one of you, I think). Any extra context/thoughts you could share here, briefly?
>> 
>> Is your argument really that we should intentionally pessimize something because the committee decided not to standardize a similar container?  LLVM's entire ADT library is basically a laundry list of micro-optimizations that we felt at some point or another had advantages over what the STL provides.
>> 
>> John.
>> 
>>> 
>>> - Dave
>>> 
>>> 
>>> Peter
>>> 
>>> 
>>> On Dec 19, 2016 20:29, "David Blaikie" <dblaikie at gmail.com> wrote:
>>> What I mean is: compare OwningArrayRef to std::vector, not OwningArrayRef to manual memory management
>>> 
>>> On Mon, Dec 19, 2016 at 8:07 PM Peter Collingbourne <peter at pcc.me.uk> wrote:
>>> I don't really have a strong opinion about it. It wraps up some manual memory management code we used to have in the vtable builder, although I couldn't say how beneficial that memory management really is.
>>> 
>>> Peter
>>> 
>>> On Mon, Dec 19, 2016 at 7:56 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>> Is this really worth having compared to std::vector? std::dynarray was rejected from standardization for that reason, if I understand/heard correctly (& OwningArrayRef seems similar to std::dynarray)
>>> 
>>> On Tue, Dec 13, 2016 at 12:34 PM Peter Collingbourne via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>>> Author: pcc
>>> Date: Tue Dec 13 14:24:24 2016
>>> New Revision: 289579
>>> 
>>> URL: http://llvm.org/viewvc/llvm-project?rev=289579&view=rev
>>> Log:
>>> ADT: Add OwningArrayRef class.
>>> 
>>> This is a MutableArrayRef that owns its array.
>>> I plan to use this in D22296.
>>> 
>>> Differential Revision: https://reviews.llvm.org/D27723
>>> 
>>> Modified:
>>>    llvm/trunk/include/llvm/ADT/ArrayRef.h
>>> 
>>> Modified: llvm/trunk/include/llvm/ADT/ArrayRef.h
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/ArrayRef.h?rev=289579&r1=289578&r2=289579&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/include/llvm/ADT/ArrayRef.h (original)
>>> +++ llvm/trunk/include/llvm/ADT/ArrayRef.h Tue Dec 13 14:24:24 2016
>>> @@ -413,6 +413,25 @@ namespace llvm {
>>>     }
>>>   };
>>> 
>>> +  /// This is a MutableArrayRef that owns its array.
>>> +  template <typename T> class OwningArrayRef : public MutableArrayRef<T> {
>>> +  public:
>>> +    OwningArrayRef() {}
>>> +    OwningArrayRef(size_t Size) : MutableArrayRef<T>(new T[Size], Size) {}
>>> +    OwningArrayRef(ArrayRef<T> Data)
>>> +        : MutableArrayRef<T>(new T[Data.size()], Data.size()) {
>>> +      std::copy(Data.begin(), Data.end(), this->begin());
>>> +    }
>>> +    OwningArrayRef(OwningArrayRef &&Other) { *this = Other; }
>>> +    OwningArrayRef &operator=(OwningArrayRef &&Other) {
>>> +      delete this->data();
>>> +      this->MutableArrayRef<T>::operator=(Other);
>>> +      Other.MutableArrayRef<T>::operator=(MutableArrayRef<T>());
>>> +      return *this;
>>> +    }
>>> +    ~OwningArrayRef() { delete this->data(); }
>>> +  };
>>> +
>>>   /// @name ArrayRef Convenience constructors
>>>   /// @{
>>> 
>>> 
>>> 
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>> 
>>> 
>>> 
>>> -- 
>>> -- 
>>> Peter
>>> 
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> 