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

[yurai007]

Summary

It's https://reviews.llvm.org/D129565 follow-up.

[yurai007]

llvm/include/llvm/ADT/SmallVector.h

1292

TODO: add more tests to make sure there is no surprises coming from overloading rules

llvm/unittests/ADT/SmallVectorTest.cpp

1139

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.

[nikic]

llvm/include/llvm/ADT/SmallVector.h

1287

Why do we need to explicitly pass InlineSize<R>::value? Isn't that the default anyway?

llvm/lib/Analysis/TypeBasedAliasAnalysis.cpp

814 ↗

(On Diff #444678)

llvm prefix unnecessary

[nikic]

By the way, I think it would be worthwhile to add an ArrayRef constructor to SmallVector, or else drop the existing iterator_range ctor. I've wanted to switch an iterator_range to ArrayRef a few times in the past, and always gave up due to the SmallVector fallout. We should support both or neither.

[dexonsmith]

In D129781#3652424, @nikic wrote:

By the way, I think it would be worthwhile to add an ArrayRef constructor to SmallVector, or else drop the existing iterator_range ctor. I've wanted to switch an iterator_range to ArrayRef a few times in the past, and always gave up due to the SmallVector fallout. We should support both or neither.

“Neither” makes some sense to me, since to_vector and to_vector_of are clear and support any range naturally. @dblaikie, any thoughts?

[dblaikie]

In D129781#3652460, @dexonsmith wrote:

In D129781#3652424, @nikic wrote:

By the way, I think it would be worthwhile to add an ArrayRef constructor to SmallVector, or else drop the existing iterator_range ctor. I've wanted to switch an iterator_range to ArrayRef a few times in the past, and always gave up due to the SmallVector fallout. We should support both or neither.

“Neither” makes some sense to me, since to_vector and to_vector_of are clear and support any range naturally. @dblaikie, any thoughts?

Yeah, I'd tend to lean that way - if std::vector doesn't have generic range-based construction, I'm happy with SmallVector not having it either.

llvm/include/llvm/ADT/SmallVector.h

1297–1300

Rather than an overload, can the Size parameter have a default value in the other definition and this overload can be removed?

[yurai007]

Remove WIP tag since overall direction seem to be fine and now all tests are passing (at least locally). Address some comments.

[yurai007]

llvm/include/llvm/ADT/SmallVector.h

1287

Yes, it's indeed redundant! Interesting, I didn't even bother thinking about removing it since it was present on to_vector before my change. And given that's about widely used SmallVector header I didn't expect that it could have anything superfluous.

[dexonsmith]

llvm/include/llvm/ADT/SmallVector.h

1287

Maybe it predates SmallVector itself having a default? Or maybe it has some hard-to-predict impact on compile time? You might be able to figure that out via a git-blame and/or a look at the original review thread, or you can ask the author that added it (if it was me, I don’t remember :)). Most likely it can just be skipped though.

[yurai007]

In D129781#3653012, @dblaikie wrote:

In D129781#3652460, @dexonsmith wrote:

In D129781#3652424, @nikic wrote:

By the way, I think it would be worthwhile to add an ArrayRef constructor to SmallVector, or else drop the existing iterator_range ctor. I've wanted to switch an iterator_range to ArrayRef a few times in the past, and always gave up due to the SmallVector fallout. We should support both or neither.

“Neither” makes some sense to me, since to_vector and to_vector_of are clear and support any range naturally. @dblaikie, any thoughts?

Yeah, I'd tend to lean that way - if std::vector doesn't have generic range-based construction, I'm happy with SmallVector not having it either.

All right, I will prepare separate change for this.

[yurai007]

llvm/include/llvm/ADT/SmallVector.h

1297–1300

The thing is we need to compute default Size from R which is Size's successor on template parameter list. We can reorder list and it would work as long as Size is default:

 template <typename Out, typename R, unsigned Size = CalculateSmallVectorDefaultInlinedElements<
             ValueTypeFromRangeType<R>>::value>
  SmallVector<Out, Size> to_vector_of(R &&Range) {
  return {std::begin(Range), std::end(Range)};
}

However when Size is non-default both preceding types Out, R need to be passed explicitly during instantiation. If I understand it correctly, that was reason for having 2 definitions of to_vector function - for similar reason they couldn't be merged into one. It would be much easier if Size depended on Out instead but except some special scenarios when it's allowed (pointer types), in general I don't think we can rely on this.

[yurai007]

Replace to_vector_of usage with to_vector where possible, improve names in ToVectorOf test case. All comments should be addressed now.

[yurai007]

Some cosmetic changes in tests.

[yurai007]

Ping

[dblaikie]

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)

llvm/include/llvm/ADT/SmallVector.h

1286–1287

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?

llvm/unittests/ADT/SmallVectorTest.cpp

1139

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?

1163–1165

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

1168–1178

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?

[hiraditya]

would it be possible to specialize cast<> for this?

[dblaikie]

In D129781#3686041, @hiraditya wrote:

would it be possible to specialize cast<> for this?

Seems like a stretch for the concept/definition of cast<> though I recall there are some novel implementations of cast for other types - so I'm open to an argument to the contrary, that this conversion would be consistent with others implemented via cast<>.

[nikic]

I think cast<> comes with the implication that the conversion is zero-cost (in non-assert builds), which is not the case here. to_vector() is an expensive O(n) operation.

[yurai007]

llvm/include/llvm/ADT/SmallVector.h

1286–1287

You are right, I can extract it to separate patch.

llvm/unittests/ADT/SmallVectorTest.cpp

1139

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.

1163–1165

Ok. I will fix it.

1168–1178

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.

[yurai007]

In D129781#3685752, @dblaikie wrote:

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)

Ok. I'm gonna split it in such way.

[yurai007]

Addressed comments.

[dblaikie]

llvm/unittests/ADT/SmallVectorTest.cpp

865–868

This could be a constexpr variable template, maybe? & then the EXPECT_TRUEs testing this could instead be static_assert rather than runtime tests?

1168–1178

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?)

1181–1183

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)?

1182–1221

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?)

[yurai007]

Address all recent comments.

[yurai007]

llvm/unittests/ADT/SmallVectorTest.cpp

1182–1221

I agree it's not necessary so eventually decided to remove ArrayRef variants.

[yurai007]

llvm/unittests/ADT/SmallVectorTest.cpp

1144

For now message argument is required, soon after https://reviews.llvm.org/D130689 could be removed. Anyway if needed may be done in separate NFC.

[yurai007]

Rebase and adjust to C++17 (static_asserts)

[dblaikie]

Looks good

[yurai007]

In D129781#3713421, @dblaikie wrote:

Looks good

@nikic @dblaikie @dexonsmith: Great. Thank you all for the review!

llvm/include/llvm/ADT/SmallVector.h

 /// SmallVector with elements of the vector.  This is useful, for example,
 /// when you want to iterate a range and then sort the results.
 template <unsigned Size, typename R>
 SmallVector<ValueTypeFromRangeType<R>, Size> to_vector(R &&Range) {
   return {std::begin(Range), std::end(Range)};
 }
 template <typename R>
 SmallVector<ValueTypeFromRangeType<R>,
             CalculateSmallVectorDefaultInlinedElements<
                 ValueTypeFromRangeType<R>>::value>
 to_vector(R &&Range) {
   return {std::begin(Range), std::end(Range)};
 }
 
+template <typename Out, unsigned Size, typename R>
+SmallVector<Out, Size> to_vector_of(R &&Range) {
+  return {std::begin(Range), std::end(Range)};
+}
+
+template <typename Out, typename R> SmallVector<Out> to_vector_of(R &&Range) {
+  return {std::begin(Range), std::end(Range)};
+}
+
 } // end namespace llvm
 
 namespace std {
 
   /// Implement std::swap in terms of SmallVector swap.
   template<typename T>
   inline void
   swap(llvm::SmallVectorImpl<T> &LHS, llvm::SmallVectorImpl<T> &RHS) {

llvm/unittests/ADT/SmallVectorTest.cpp

 
 TYPED_TEST(SmallVectorTest, IteratorTest) {
   std::list<int> L;
   this->theVector.insert(this->theVector.end(), L.begin(), L.end());
 }
 
 template <typename InvalidType> class DualSmallVectorsTest;
 
+template <typename T, unsigned N>
+static unsigned NumBuiltinElts(const SmallVector<T, N> &) {
+  return N;
+}
+
 template <typename VectorT1, typename VectorT2>
 class DualSmallVectorsTest<std::pair<VectorT1, VectorT2>> : public SmallVectorTestBase {
 protected:
   VectorT1 theVector;
   VectorT2 otherVector;
-
-  template <typename T, unsigned N>
-  static unsigned NumBuiltinElts(const SmallVector<T, N>&) { return N; }
 };
 
 typedef ::testing::Types<
     // Small mode -> Small mode.
     std::pair<SmallVector<Constructable, 4>, SmallVector<Constructable, 4>>,
     // Small mode -> Big mode.
     std::pair<SmallVector<Constructable, 4>, SmallVector<Constructable, 2>>,
     // Big mode -> Small mode.
TYPED_TEST(DualSmallVectorsTest, MoveAssignment) {
   // Make sure the # of constructor/destructor calls line up. There
   // are two live objects after clearing the other vector.
   this->otherVector.clear();
   EXPECT_EQ(Constructable::getNumConstructorCalls()-4,
             Constructable::getNumDestructorCalls());
 
   // If the source vector (otherVector) was in small-mode, assert that we just
   // moved the data pointer over.
-  EXPECT_TRUE(this->NumBuiltinElts(this->otherVector) == 4 ||
+  EXPECT_TRUE(NumBuiltinElts(this->otherVector) == 4 ||
               this->theVector.data() == OrigDataPtr);
 
   // There shouldn't be any live objects any more.
   this->theVector.clear();
   EXPECT_EQ(Constructable::getNumConstructorCalls(),
             Constructable::getNumDestructorCalls());
 
   // We shouldn't have copied anything in this whole process.
TEST(SmallVectorTest, InitializerList) {
   V2.assign({4});
   EXPECT_TRUE(makeArrayRef(V2).equals({4}));
   V2.append({3, 2});
   EXPECT_TRUE(makeArrayRef(V2).equals({4, 3, 2}));
   V2.insert(V2.begin() + 1, 5);
   EXPECT_TRUE(makeArrayRef(V2).equals({4, 5, 3, 2}));
 }
 
+TEST(SmallVectorTest, ToVector) {
+  {
+    std::vector<char> v = {'a', 'b', 'c'};
+    auto Vector = to_vector<4>(v);
+    ASSERT_EQ(4u, NumBuiltinElts(Vector));
+    ASSERT_EQ(3u, Vector.size());
+    for (size_t I = 0; I < v.size(); ++I) {
+      EXPECT_EQ(v[I], Vector[I]);
+    }
+  }
+  {
+    std::vector<char> v = {'a', 'b', 'c'};
+    auto Vector = to_vector(v);
+    ASSERT_NE(4u, NumBuiltinElts(Vector));
+    ASSERT_EQ(3u, Vector.size());
+    for (size_t I = 0; I < v.size(); ++I) {
+      EXPECT_EQ(v[I], Vector[I]);
+    }
+  }
+}
+
+TEST(SmallVectorTest, ToVectorOf) {
+  struct From {
+    int Content;
+    friend bool operator==(const From &LHS, const From &RHS) {
+      return LHS.Content == RHS.Content;
+    }
+  };
+
+  class To {
+  public:
+    To() = default;
+    To(From *M) { F = M; }
+    operator From *() const { return get(); }
+    From &operator*() const { return *get(); }
+
+  private:
+    From *get() const { return F; }
+    From *F = nullptr;
+  };
+
+  From from1{1}, from2{2}, from3{3};
+  std::vector<To> StdVector = {To(&from1), To(&from2), To(&from3)};
+  {
+    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]);
+    }
+  }
+  {
+    ArrayRef<To> Array = StdVector;
+    llvm::SmallVector<From *> Vector = llvm::to_vector_of<From *>(Array);
+
+    ASSERT_EQ(Array.size(), Vector.size());
+    for (size_t I = 0; I < Array.size(); ++I) {
+      EXPECT_EQ(Array[I], Vector[I]);
+      EXPECT_EQ(*Array[I], *Vector[I]);
+    }
+  }
+  {
+    auto Vector = llvm::to_vector_of<From *, 4>(StdVector);
+
+    ASSERT_EQ(StdVector.size(), Vector.size());
+    ASSERT_EQ(4u, NumBuiltinElts(Vector));
+    for (size_t I = 0; I < StdVector.size(); ++I) {
+      EXPECT_EQ(StdVector[I], Vector[I]);
+      EXPECT_EQ(*StdVector[I], *Vector[I]);
+    }
+  }
+  {
+    ArrayRef<To> Array = StdVector;
+    auto Vector = llvm::to_vector_of<From *, 4>(Array);
+
+    ASSERT_EQ(Array.size(), Vector.size());
+    ASSERT_EQ(4u, NumBuiltinElts(Vector));
+    for (size_t I = 0; I < Array.size(); ++I) {
+      EXPECT_EQ(Array[I], Vector[I]);
+      EXPECT_EQ(*Array[I], *Vector[I]);
+    }
+  }
+}
+
 template <class VectorT>
 class SmallVectorReferenceInvalidationTest : public SmallVectorTestBase {
 protected:
   const char *AssertionMessage =
       "Attempting to reference an element of the vector in an operation \" "
       "\"that invalidates it";
 
   VectorT V;
 
-  template <typename T, unsigned N>
-  static unsigned NumBuiltinElts(const SmallVector<T, N> &) {
-    return N;
-  }
-
   template <class T> static bool isValueType() {
     return std::is_same<T, typename VectorT::value_type>::value;
   }
 
   void SetUp() override {
     SmallVectorTestBase::SetUp();
 
     // Fill up the small size so that insertions move the elements.
using SmallVectorReferenceInvalidationTestTypes =
     ::testing::Types<SmallVector<int, 3>, SmallVector<Constructable, 3>>;
 
 TYPED_TEST_SUITE(SmallVectorReferenceInvalidationTest,
                  SmallVectorReferenceInvalidationTestTypes, );
 
 TYPED_TEST(SmallVectorReferenceInvalidationTest, PushBack) {
   // Note: setup adds [1, 2, ...] to V until it's at capacity in small mode.
   auto &V = this->V;
-  int N = this->NumBuiltinElts(V);
+  int N = NumBuiltinElts(V);
 
   // Push back a reference to last element when growing from small storage.
   V.push_back(V.back());
   EXPECT_EQ(N, V.back());
 
   // Check that the old value is still there (not moved away).
   EXPECT_EQ(N, V[V.size() - 2]);
 
   // Fill storage again.
   V.back() = V.size();
   while (V.size() < V.capacity())
     V.push_back(V.size() + 1);
 
   // Push back a reference to last element when growing from large storage.
   V.push_back(V.back());
   EXPECT_EQ(int(V.size()) - 1, V.back());
 }
 
 TYPED_TEST(SmallVectorReferenceInvalidationTest, PushBackMoved) {
   // Note: setup adds [1, 2, ...] to V until it's at capacity in small mode.
   auto &V = this->V;
-  int N = this->NumBuiltinElts(V);
+  int N = NumBuiltinElts(V);
 
   // Push back a reference to last element when growing from small storage.
   V.push_back(std::move(V.back()));
   EXPECT_EQ(N, V.back());
   if (this->template isValueType<Constructable>()) {
     // Check that the value was moved (not copied).
     EXPECT_EQ(0, V[V.size() - 2]);
   }
if (this->template isValueType<Constructable>()) {
     // Check the value got moved out.
     EXPECT_EQ(0, V[V.size() - 2]);
   }
 }
 
 TYPED_TEST(SmallVectorReferenceInvalidationTest, Resize) {
   auto &V = this->V;
   (void)V;
-  int N = this->NumBuiltinElts(V);
+  int N = NumBuiltinElts(V);
   V.resize(N + 1, V.back());
   EXPECT_EQ(N, V.back());
 
   // Resize to add enough elements that V will grow again. If reference
   // invalidation breaks in the future, sanitizers should be able to catch a
   // use-after-free here.
   V.resize(V.capacity() + 1, V.front());
   EXPECT_EQ(1, V.back());
 }
 
 TYPED_TEST(SmallVectorReferenceInvalidationTest, Append) {
   auto &V = this->V;
   (void)V;
   V.append(1, V.back());
-  int N = this->NumBuiltinElts(V);
+  int N = NumBuiltinElts(V);
   EXPECT_EQ(N, V[N - 1]);
 
   // Append enough more elements that V will grow again. This tests growing
   // when already in large mode.
   //
   // If reference invalidation breaks in the future, sanitizers should be able
   // to catch a use-after-free here.
   V.append(V.capacity() - V.size() + 1, V.front());
   EXPECT_EQ(1, V.back());
 }
 
 TYPED_TEST(SmallVectorReferenceInvalidationTest, AppendRange) {
   auto &V = this->V;
   (void)V;
 #if !defined(NDEBUG) && GTEST_HAS_DEATH_TEST
   EXPECT_DEATH(V.append(V.begin(), V.begin() + 1), this->AssertionMessage);
 
-  ASSERT_EQ(3u, this->NumBuiltinElts(V));
+  ASSERT_EQ(3u, NumBuiltinElts(V));
   ASSERT_EQ(3u, V.size());
   V.pop_back();
   ASSERT_EQ(2u, V.size());
 
   // Confirm this checks for growth when there's more than one element
   // appended.
   EXPECT_DEATH(V.append(V.begin(), V.end()), this->AssertionMessage);
 #endif
 }
 
 TYPED_TEST(SmallVectorReferenceInvalidationTest, Assign) {
   // Note: setup adds [1, 2, ...] to V until it's at capacity in small mode.
   auto &V = this->V;
   (void)V;
-  int N = this->NumBuiltinElts(V);
+  int N = NumBuiltinElts(V);
   ASSERT_EQ(unsigned(N), V.size());
   ASSERT_EQ(unsigned(N), V.capacity());
 
   // Check assign that shrinks in small mode.
   V.assign(1, V.back());
   EXPECT_EQ(1u, V.size());
   EXPECT_EQ(N, V[0]);
 

 }
 
 TYPED_TEST(SmallVectorReferenceInvalidationTest, InsertN) {
   auto &V = this->V;
   (void)V;
 
   // Cover NumToInsert <= this->end() - I.
   V.insert(V.begin() + 1, 1, V.back());
-  int N = this->NumBuiltinElts(V);
+  int N = NumBuiltinElts(V);
   EXPECT_EQ(N, V[1]);
 
   // Cover NumToInsert > this->end() - I, inserting enough elements that V will
   // also grow again; V.capacity() will be more elements than necessary but
   // it's a simple way to cover both conditions.
   //
   // If reference invalidation breaks in the future, sanitizers should be able
   // to catch a use-after-free here.
   V.insert(V.begin(), V.capacity(), V.front());
   EXPECT_EQ(1, V.front());
 }
 
 TYPED_TEST(SmallVectorReferenceInvalidationTest, InsertRange) {
   auto &V = this->V;
   (void)V;
 #if !defined(NDEBUG) && GTEST_HAS_DEATH_TEST
   EXPECT_DEATH(V.insert(V.begin(), V.begin(), V.begin() + 1),
                this->AssertionMessage);
 
-  ASSERT_EQ(3u, this->NumBuiltinElts(V));
+  ASSERT_EQ(3u, NumBuiltinElts(V));
   ASSERT_EQ(3u, V.size());
   V.pop_back();
   ASSERT_EQ(2u, V.size());
 
   // Confirm this checks for growth when there's more than one element
   // inserted.
   EXPECT_DEATH(V.insert(V.begin(), V.begin(), V.end()), this->AssertionMessage);
 #endif
 }
 
 TYPED_TEST(SmallVectorReferenceInvalidationTest, EmplaceBack) {
   // Note: setup adds [1, 2, ...] to V until it's at capacity in small mode.
   auto &V = this->V;
-  int N = this->NumBuiltinElts(V);
+  int N = NumBuiltinElts(V);
 
   // Push back a reference to last element when growing from small storage.
   V.emplace_back(V.back());
   EXPECT_EQ(N, V.back());
 
   // Check that the old value is still there (not moved away).
   EXPECT_EQ(N, V[V.size() - 2]);
 
class SmallVectorInternalReferenceInvalidationTest
     : public SmallVectorTestBase {
 protected:
   const char *AssertionMessage =
       "Attempting to reference an element of the vector in an operation \" "
       "\"that invalidates it";
 
   VectorT V;
 
-  template <typename T, unsigned N>
-  static unsigned NumBuiltinElts(const SmallVector<T, N> &) {
-    return N;
-  }
-
   void SetUp() override {
     SmallVectorTestBase::SetUp();
 
     // Fill up the small size so that insertions move the elements.
     for (int I = 0, E = NumBuiltinElts(V); I != E; ++I)
       V.emplace_back(I + 1, I + 1);
   }
 };
 
 // Test pairs of the same types from SmallVectorReferenceInvalidationTestTypes.
 using SmallVectorInternalReferenceInvalidationTestTypes =
     ::testing::Types<SmallVector<std::pair<int, int>, 3>,
                      SmallVector<std::pair<Constructable, Constructable>, 3>>;
 
 TYPED_TEST_SUITE(SmallVectorInternalReferenceInvalidationTest,
                  SmallVectorInternalReferenceInvalidationTestTypes, );
 
 TYPED_TEST(SmallVectorInternalReferenceInvalidationTest, EmplaceBack) {
   // Note: setup adds [1, 2, ...] to V until it's at capacity in small mode.
   auto &V = this->V;
-  int N = this->NumBuiltinElts(V);
+  int N = NumBuiltinElts(V);
 
   // Push back a reference to last element when growing from small storage.
   V.emplace_back(V.back().first, V.back().second);
   EXPECT_EQ(N, V.back().first);
   EXPECT_EQ(N, V.back().second);
 
   // Check that the old value is still there (not moved away).
   EXPECT_EQ(N, V[V.size() - 2].first);