barannikov88 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); ---------------- yurai007 wrote: > yurai007 wrote: > > 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. > @barannikov88: after addressing @nikic comment there is no API breaking > change anymore. As far I can tell SmallVector API is only extended and still > backward-compatible. Hope it looks better now. Looks good, thank you. Still, I would suggest requesting a review from more experienced developers than me. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130268/new/ https://reviews.llvm.org/D130268 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
