[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 inline comments.


================
Comment at: llvm/unittests/ADT/SmallVectorTest.cpp:865-868
+template <typename T, unsigned N>
+static unsigned NumBuiltinElts(const SmallVector<T, N> &) {
+  return N;
+}
----------------
This could be a constexpr variable template, maybe? & then the `EXPECT_TRUE`s testing this could instead be static_assert rather than runtime tests?


================
Comment at: llvm/unittests/ADT/SmallVectorTest.cpp:1181-1183
+  std::vector<To> StdVector = {To(&from1), To(&from2), To(&from3)};
+  {
+    llvm::SmallVector<From *> Vector = llvm::to_vector_of<From *>(StdVector);
----------------
Oh, looks like this is backwards, maybe? I'd expect the source type to be `To` and the destination type to be `From`, so more like `to_vector_of<To>(SomeFromVector)`?


================
Comment at: llvm/unittests/ADT/SmallVectorTest.cpp:1182-1221
+  {
+    llvm::SmallVector<From *> Vector = llvm::to_vector_of<From *>(StdVector);
+
+    ASSERT_EQ(StdVector.size(), Vector.size());
+    for (size_t I = 0; I < StdVector.size(); ++I) {
+      EXPECT_EQ(StdVector[I], Vector[I]);
+      EXPECT_EQ(*StdVector[I], *Vector[I]);
----------------
I guess this is how the other to_vectors were already tested? But if you're up for it in a pre or post commit, might be nice to simplify this - like I'm not sure it's necessary to test all these variants. The implementation is generic - it tests anything you can do begin(x) and end(x) on, so we probably only need to test one such type? But if we really want to test 4, perhaps we can avoid rewriting the code and use a type expansion in gunit?)


================
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(); }
+
----------------
yurai007 wrote:
> dblaikie wrote:
> > 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?
> I thought there would be some value in referring to MDOperand/Metadata classes since that was background for this patch: https://reviews.llvm.org/D129565#3648665 But I get your point - what matters here is just property of MDOperand being convertible to Metadata. I can use 'From' and 'To' names as more appropriate in expressing conversion.
> Short information about original, motivational use case can be simply put in 'Summary'/commit message.
Yep, that's the thinking!

Maybe we can even remove the fact that one side of this is a pointer, and the other a value? Have both sides be values (eg: `SmallVector<From>` -> `SmallVector<To>`)? Shouldn't be any need for so many operations on `From` or `To` either (eg: the `operator *`, `operator From *` (I'd expect the only conversion should be the `To(From)` ctor), `get()`, etc) - probably `From` and `To` can be structs, maybe with a single `int` member that gets copied on conversion, so that can be checked on the other side of the conversion?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129781