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

[Dawid Jurczak via Phabricator via cfe-commits]

yurai007 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:
> barannikov88 wrote:
> > nikic wrote:
> > > 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.
> > Please also consider the fact that this is API breaking change due to this "conditional expression is amiguous" error. Many external projects depend on LLVM/ADT, and all of them will have to adapt this change.
> > 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. 
> 
> Ok, makes sense.
>     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?

You are right, explicit works when we use brackets instead assignment. I'm gonna add it to constructor and adjust rest of patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130268