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

[Dawid Jurczak via Phabricator via llvm-commits]

yurai007 added inline comments.


================
Comment at: llvm/include/llvm/ADT/SmallVector.h:1286-1287
-SmallVector<ValueTypeFromRangeType<R>,
-            CalculateSmallVectorDefaultInlinedElements<
-                ValueTypeFromRangeType<R>>::value>
-to_vector(R &&Range) {
----------------
dblaikie wrote:
> 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`?
You are right, I can extract it to separate patch.


================
Comment at: llvm/unittests/ADT/SmallVectorTest.cpp:1163-1165
+    bool operator==(const Metadata &RHS) const {
+      return Content == RHS.Content;
+    }
----------------
dblaikie wrote:
> 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
Ok. I will fix it.


================
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(); }
+
----------------
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.


================
Comment at: llvm/unittests/ADT/SmallVectorTest.cpp:1137
 
+TEST(SmallVectorTest, ToVector) {
+  {
----------------
dblaikie wrote:
> 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?
> 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?

I would agree except the fact that STLExtrasTest.ToVector is not exactly 1-1 copy because it has enumerate() call (STLExtras.h) and I believe it tests both to_vector and enumerate functions. There are following options I see right now:
1. We can cut and paste STLExtrasTest.ToVector here without any modification (well, except test case name), but it requires extra include for enumerate().  That enumerate is not really needed for purpose of testing to_vector function. 
2. We can simply remove STLExtrasTest.ToVector and leave SmallVectorTest.ToVector as it is but I'm a bit paranoid about removing unit test covering enumerate since it smells like regression.
3. We can leave STLExtrasTest.ToVector  as it is, and copy it to SmallVectorTest.ToVector with enumerate (because it's irrelevant) removal. That's chosen option. 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129781