[PATCH] D31058: Store Arguments in a flat array instead of an iplist

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: lib/IR/Function.cpp:230-231
+  if (NumArgs > 0) {
+    Arguments =
+        reinterpret_cast<Argument *>(malloc(sizeof(Argument) * NumArgs));
+    for (unsigned i = 0, e = NumArgs; i != e; ++i) {
----------------
chandlerc wrote:
> rnk wrote:
> > rnk wrote:
> > > chandlerc wrote:
> > > > Why malloc?
> > > Is there a way to use 'new' to allocate uninitialized memory suitable for this kind of placement new construction? I can do `new char[sizeof(Argument)*NumArgs]`, but it didn't seem prettier. Regular array new wants a default ctor, which Argument doesn't have, and adding one would be wasteful. I just want to allocate some memory and initialize some complex, non-value type objects into it. How hard can that be? ¯\_(ツ)_/¯
> > Maybe you're suggesting `::operator new(...)`? That'd work.
> Is `std::allocator<Argument>().allocate(NumArgs)` too verbose/arcane? It has the semantics I think we want here. (And `.deallocate(...)' below.)
> 
> Anyways, I don't feel particularly strongly about this.
Nope, sounds good, gets rid of the reinterpret_cast. Thanks!


================
Comment at: lib/IR/Function.cpp:245-247
+static MutableArrayRef<Argument> makeArgArray(Argument *Args, size_t Count) {
+  return MutableArrayRef<Argument>(Args, Count);
+}
----------------
chandlerc wrote:
> We should either have `makeArrayRef` produce a MutableArrayRef for non-const pointers or we should add a `makeMutableArrayRef` that accepts non-const pointers. Then I don't think you'll need this.
> 
> That can bea  follow-up cleanup.
I think we want the second, or we'll need to add a conversion from MutableArrayRef<T> to ArrayRef<T> to avoid breaking callers of makeArrayRef with non-const pointers. MutableArrayRef is pretty niche, though, so I'm gonna leave it alone for now.


https://reviews.llvm.org/D31058