ADT: Use 'using' to inherit assign and append in SmallString

Rather than reimplement, use a `using` declaration to bring in
`SmallVectorImpl<char>`'s assign and append implementations in
`SmallString`.

The `SmallString` versions were missing reference invalidation
assertions from `SmallVector`. This patch also fixes a bug in
`llvm::FileCollector::addFileImpl`, which was a copy/paste from
`clang::ModuleDependencyCollector::copyToRoot`, both caught by the
no-longer-skipped assertions.

As a drive-by, this also sinks the `const SmallVectorImpl&` versions of
these methods down into `SmallVectorImpl`, since I imagine they'd be
useful elsewhere.

Differential Revision: https://reviews.llvm.org/D95202

Author: dexonsmith

clang/lib/Frontend/ModuleDependencyCollector.cpp

@@ -190,17 +190,17 @@ std::error_code ModuleDependencyCollector::copyToRoot(StringRef Src,
   // Canonicalize src to a native path to avoid mixed separator styles.
   path::native(AbsoluteSrc);
   // Remove redundant leading "./" pieces and consecutive separators.
-  AbsoluteSrc = path::remove_leading_dotslash(AbsoluteSrc);
+  StringRef TrimmedAbsoluteSrc = path::remove_leading_dotslash(AbsoluteSrc);
 
   // Canonicalize the source path by removing "..", "." components.
-  SmallString<256> VirtualPath = AbsoluteSrc;
+  SmallString<256> VirtualPath = TrimmedAbsoluteSrc;
   path::remove_dots(VirtualPath, /*remove_dot_dot=*/true);
 
   // If a ".." component is present after a symlink component, remove_dots may
   // lead to the wrong real destination path. Let the source be canonicalized
   // like that but make sure we always use the real path for the destination.
   SmallString<256> CopyFrom;
-  if (!getRealPath(AbsoluteSrc, CopyFrom))
+  if (!getRealPath(TrimmedAbsoluteSrc, CopyFrom))
     CopyFrom = VirtualPath;
   SmallString<256> CacheDst = getDest();
 

llvm/include/llvm/ADT/SmallString.h

@@ -40,35 +40,15 @@ class SmallString : public SmallVector<char, InternalLen> {
   template<typename ItTy>
   SmallString(ItTy S, ItTy E) : SmallVector<char, InternalLen>(S, E) {}
 
-  // Note that in order to add new overloads for append & assign, we have to
-  // duplicate the inherited versions so as not to inadvertently hide them.
-
   /// @}
   /// @name String Assignment
   /// @{
 
-  /// Assign from a repeated element.
-  void assign(size_t NumElts, char Elt) {
-    this->SmallVectorImpl<char>::assign(NumElts, Elt);
-  }
-
-  /// Assign from an iterator pair.
-  template<typename in_iter>
-  void assign(in_iter S, in_iter E) {
-    this->clear();
-    SmallVectorImpl<char>::append(S, E);
-  }
+  using SmallVector<char, InternalLen>::assign;
 
   /// Assign from a StringRef.
   void assign(StringRef RHS) {
-    this->clear();
-    SmallVectorImpl<char>::append(RHS.begin(), RHS.end());
-  }
-
-  /// Assign from a SmallVector.
-  void assign(const SmallVectorImpl<char> &RHS) {
-    this->clear();
-    SmallVectorImpl<char>::append(RHS.begin(), RHS.end());
+    SmallVectorImpl<char>::assign(RHS.begin(), RHS.end());
   }
 
   /// Assign from a list of StringRefs.
@@ -81,26 +61,13 @@ class SmallString : public SmallVector<char, InternalLen> {
   /// @name String Concatenation
   /// @{
 
-  /// Append from an iterator pair.
-  template<typename in_iter>
-  void append(in_iter S, in_iter E) {
-    SmallVectorImpl<char>::append(S, E);
-  }
-
-  void append(size_t NumInputs, char Elt) {
-    SmallVectorImpl<char>::append(NumInputs, Elt);
-  }
+  using SmallVector<char, InternalLen>::append;
 
   /// Append from a StringRef.
   void append(StringRef RHS) {
     SmallVectorImpl<char>::append(RHS.begin(), RHS.end());
   }
 
-  /// Append from a SmallVector.
-  void append(const SmallVectorImpl<char> &RHS) {
-    SmallVectorImpl<char>::append(RHS.begin(), RHS.end());
-  }
-
   /// Append from a list of StringRefs.
   void append(std::initializer_list<StringRef> Refs) {
     size_t SizeNeeded = this->size();

llvm/include/llvm/ADT/SmallVector.h

@@ -664,6 +664,8 @@ class SmallVectorImpl : public SmallVectorTemplateBase<T> {
     append(IL.begin(), IL.end());
   }
 
+  void append(const SmallVectorImpl &RHS) { append(RHS.begin(), RHS.end()); }
+
   void assign(size_type NumElts, ValueParamT Elt) {
     // Note that Elt could be an internal reference.
     if (NumElts > this->capacity()) {
@@ -698,6 +700,8 @@ class SmallVectorImpl : public SmallVectorTemplateBase<T> {
     append(IL);
   }
 
+  void assign(const SmallVectorImpl &RHS) { assign(RHS.begin(), RHS.end()); }
+
   iterator erase(const_iterator CI) {
     // Just cast away constness because this is a non-const member function.
     iterator I = const_cast<iterator>(CI);

llvm/lib/Support/FileCollector.cpp

@@ -86,17 +86,18 @@ void FileCollector::addFileImpl(StringRef SrcPath) {
   sys::path::native(AbsoluteSrc);
 
   // Remove redundant leading "./" pieces and consecutive separators.
-  AbsoluteSrc = sys::path::remove_leading_dotslash(AbsoluteSrc);
+  StringRef TrimmedAbsoluteSrc =
+      sys::path::remove_leading_dotslash(AbsoluteSrc);
 
   // Canonicalize the source path by removing "..", "." components.
-  SmallString<256> VirtualPath = AbsoluteSrc;
+  SmallString<256> VirtualPath = TrimmedAbsoluteSrc;
   sys::path::remove_dots(VirtualPath, /*remove_dot_dot=*/true);
 
   // If a ".." component is present after a symlink component, remove_dots may
   // lead to the wrong real destination path. Let the source be canonicalized
   // like that but make sure we always use the real path for the destination.
   SmallString<256> CopyFrom;
-  if (!getRealPath(AbsoluteSrc, CopyFrom))
+  if (!getRealPath(TrimmedAbsoluteSrc, CopyFrom))
     CopyFrom = VirtualPath;
 
   SmallString<256> DstPath = StringRef(Root);

llvm/unittests/ADT/SmallVectorTest.cpp

@@ -485,6 +485,15 @@ TYPED_TEST(SmallVectorTest, AppendRepeatedNonForwardIterator) {
   this->assertValuesInOrder(this->theVector, 3u, 1, 7, 7);
 }
 
+TYPED_TEST(SmallVectorTest, AppendSmallVector) {
+  SCOPED_TRACE("AppendSmallVector");
+
+  SmallVector<Constructable, 3> otherVector = {7, 7};
+  this->theVector.push_back(Constructable(1));
+  this->theVector.append(otherVector);
+  this->assertValuesInOrder(this->theVector, 3u, 1, 7, 7);
+}
+
 // Assign test
 TYPED_TEST(SmallVectorTest, AssignTest) {
   SCOPED_TRACE("AssignTest");
@@ -513,6 +522,15 @@ TYPED_TEST(SmallVectorTest, AssignNonIterTest) {
   this->assertValuesInOrder(this->theVector, 2u, 7, 7);
 }
 
+TYPED_TEST(SmallVectorTest, AssignSmallVector) {
+  SCOPED_TRACE("AssignSmallVector");
+
+  SmallVector<Constructable, 3> otherVector = {7, 7};
+  this->theVector.push_back(Constructable(1));
+  this->theVector.assign(otherVector);
+  this->assertValuesInOrder(this->theVector, 2u, 7, 7);
+}
+
 // Move-assign test
 TYPED_TEST(SmallVectorTest, MoveAssignTest) {
   SCOPED_TRACE("MoveAssignTest");