[PATCH] D130268: [WIP] Add SmallVector constructor to allow creation of SmallVector<T> from ArrayRef of items convertible to type T

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: clang/lib/CodeGen/CGExprConstant.cpp:504
   llvm::StructType *STy = llvm::ConstantStruct::getTypeForElements(
-      CGM.getLLVMContext(), Packed ? PackedElems : UnpackedElems, Packed);
+      CGM.getLLVMContext(), Packed ? PackedElems : to_vector(UnpackedElems),
+      Packed);
----------------
barannikov88 wrote:
> yurai007 wrote:
> > nikic wrote:
> > > yurai007 wrote:
> > > > That's because of "error: conditional expression is ambiguous; 'llvm::SmallVector<llvm::Constant *, 32>' can be converted to 'ArrayRef<llvm::Constant *>' and vice versa". Need to check if there is easier workaround.
> > > Would making the ctor explicit help?
> > Nope :( Making constructor explicit disables implicit conversion so we cannot do things like: SmallVector<int, 16> NewMask = Mask; anymore.
> And leaving it implicit hides the fact of possible memory allocation, which is not cheap. I think absense of such constructor is by-design.
> 
> Making it explicit is making it redundant, because there is already a constructor which accepts begin / end, and one that accepts range (note that it is explicit!). It won't save on typing, either.
> 
> I'm not in favor of this patch, but my word does not count much, so I won't block it. I'd suggest you, however, to request review of core maintainers.
> Nope :( Making constructor explicit disables implicit conversion so we cannot do things like: SmallVector<int, 16> NewMask = Mask; anymore.

I think that's fine. Similar to the existing iterator_range constructor, we would require `SmallVector<int, 16> NewMask(Mask)`, which seems like the idiomatic way to write it anyway?

> Making it explicit is making it redundant, because there is already a constructor which accepts begin / end, and one that accepts range (note that it is explicit!). It won't save on typing, either.

It is not redundant. It ensures that iterator_range and ArrayRef can be freely substituted. Switching iterator_range to ArrayRef currently requires going through a lot of SmallVector constructors and replacing them with less readable code. The alternative to this change is D129988, which looks like a significant regression in code quality.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130268/new/

https://reviews.llvm.org/D130268