[PATCH] D129781: [NFC] Introduce llvm::to_vector_of to allow creation of SmallVector<T> from range of items convertible to type T

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

Might be worth submitting this separately/without updating all the callers - so that if a caller has to be reverted for some reason we don't have to revert the functionality and testing, etc, itself? (callers could probably done in reasonable chunks - 10s, maybe 50 at a time? huge patches are a bit awkward (again, in terms of bisecting/reverting/etc) but doing one patch for each caller's probably excessive too - somewhere in between)



================
Comment at: llvm/include/llvm/ADT/SmallVector.h:1286-1287
-SmallVector<ValueTypeFromRangeType<R>,
-            CalculateSmallVectorDefaultInlinedElements<
-                ValueTypeFromRangeType<R>>::value>
-to_vector(R &&Range) {
----------------
Patch might be easier to read if these were removed in a separate patch, if they can be separate? (looks like they should be/are unrelated to the addition of `to_vector_of`?


================
Comment at: llvm/unittests/ADT/SmallVectorTest.cpp:1163-1165
+    bool operator==(const Metadata &RHS) const {
+      return Content == RHS.Content;
+    }
----------------
Generally prefer non-member overloads where possible to allow for equal conversions for LHS and RHS. In this case it could be a friend to make it easy to write inline


================
Comment at: llvm/unittests/ADT/SmallVectorTest.cpp:1168-1178
+  class MDOperand {
+  public:
+    MDOperand() = default;
+    MDOperand(Metadata *M) { MD = M; }
+    operator Metadata *() const { return get(); }
+    Metadata &operator*() const { return *get(); }
+
----------------
This seems a bit too specific? Emulating a particular LLVM data type like MDOperand - could this be named/designed more generally/abstractly so as not to be overloaded with the specifics of some LLVM detail?


================
Comment at: llvm/unittests/ADT/SmallVectorTest.cpp:1137
 
+TEST(SmallVectorTest, ToVector) {
+  {
----------------
yurai007 wrote:
> That's basically copy of STLExtrasTest.ToVector test body after removing enumerate() calls. I feel it make sense to have it here since to_vector belongs to SmallVector. 
We shouldn't duplicate tests - if to_vector is in SmallVector.h, it should be tested in SmallVectorTest.cpp, not in STLExtrasTest.cpp - so it should probably be moved here, rather than copied - and that could be done in a preliminary/separate commit?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129781