[PATCH] D129565: [NFC][Metadata] Change MDNode::operands()'s return type from op_range to ArrayRef<MDOperand>

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith accepted this revision.
dexonsmith added a comment.
This revision is now accepted and ready to land.

This looks good to me, but it’d be nice to clean up further somehow later, maybe by adding a new free function.



================
Comment at: llvm/include/llvm/IR/Metadata.h:1348
   TempMDTuple cloneImpl() const {
-    return getTemporary(getContext(), SmallVector<Metadata *, 4>(operands()));
   }
----------------
yurai007 wrote:
> dexonsmith wrote:
> > I’m surprised SmallVector doesn’t have a generic constructor from ArrayRef. I wonder if it should?
> > 
> > (why did the iterator_range work before, but ArrayRef doesn’t?)
> That's because SmallVector has appropriate constructor:
>   
>   template <typename RangeTy>
>   explicit SmallVector(const iterator_range<RangeTy> &R);
>   
> but is lacking one for ArrayRef. However from what I can see by browsing containers definitions in llvm/ADT, convenience constructors taking ArrayRefs are not very popular. There is only TinyPtrVector which has one. But the same can be said for iterator_range - only SmallVector constructor takes it.
> Even if constructing SmallVector from ArrayRef's iterators seem to not be extremely popular, still, I can find some places in code base (except ones from this change ofc. ) that would benefit from having convenience constructor - files where ArrayRef mask is used like X86ISelLowering.cpp, HexagonISelDAGToDAGHVX.cpp and DAGCombiner.cpp.
I was wondering if `to_vector<Metadata *>(operands())` would work here, but it doesn’t have a template for the value type. We’d need a new thing, `to_vector_of<>`, which took the desired value type as the first parameter. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129565