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

[dexonsmith]

Summary

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.

[dblaikie]

Looks good - some optional feedback on the code fixes. Please run clang-format on the changes here (phab lint picked up some cases in the test code that could be cleaned up).

clang/lib/Frontend/ModuleDependencyCollector.cpp

196

Maybe just call this inconditionally - it's probably cheap enough when the range is zero-length?

Perhaps it'd be more readable if "remove_leading_dotslash" had a version that could return the prefix length to remove.

But also, this is ultimately passed to 'getRealPath' which takes a StringRef, so there's technically no need to rewrite this SmallString - the StringRef from remove_leading_dotslash could be passed instead, something like:

StringRef TrimmedAbsoluteSrc = path::remove_leading_dotslash(AbsoluteSrc);
...
if (!getRealPath(AbsoluteSrc, CopyFrom))
...

llvm/lib/Support/FileCollector.cpp

89–93

Similar comment here as with ModuleDependencyCollector (actually there's a bunch of similar code here - perhaps it could be deduplicated in some way)

[dexonsmith]

Thanks for the review! Pushed with suggestions applied in ba5628f2c2a9de049b80b3e276f7e05f481c49e7.

llvm/lib/Support/FileCollector.cpp

89–93

Yeah, I may follow up to fix that. It looks like a pure copy/paste...

clang/lib/Frontend/ModuleDependencyCollector.cpp

std::error_code ModuleDependencyCollector::copyToRoot(StringRef Src,
   using namespace llvm::sys;
 
   // We need an absolute src path to append to the root.
   SmallString<256> AbsoluteSrc = Src;
   fs::make_absolute(AbsoluteSrc);
   // 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 DroppedDotSlash = path::remove_leading_dotslash(AbsoluteSrc);
+    if (DroppedDotSlash.begin() != AbsoluteSrc.begin())
+      AbsoluteSrc.erase(AbsoluteSrc.begin(), DroppedDotSlash.begin());
+  }
 
   // Canonicalize the source path by removing "..", "." components.
   SmallString<256> VirtualPath = AbsoluteSrc;
   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.

llvm/include/llvm/ADT/SmallString.h

SmallString(std::initializer_list<StringRef> Refs)
       : SmallVector<char, InternalLen>() {
     this->append(Refs);
   }
 
   /// Initialize with a range.
   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.
   void assign(std::initializer_list<StringRef> Refs) {
     this->clear();
     append(Refs);
   }
 
   /// @}
   /// @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();
     for (const StringRef &Ref : Refs)
       SizeNeeded += Ref.size();
     this->reserve(SizeNeeded);
     auto CurEnd = this->end();
     for (const StringRef &Ref : Refs) {

llvm/include/llvm/ADT/SmallVector.h

void append(size_type NumInputs, ValueParamT Elt) {
     std::uninitialized_fill_n(this->end(), NumInputs, *EltPtr);
     this->set_size(this->size() + NumInputs);
   }
 
   void append(std::initializer_list<T> IL) {
     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()) {
       this->growAndAssign(NumElts, Elt);
       return;
     }
 
     // Assign over existing elements.
void assign(in_iter in_start, in_iter in_end) {
     append(in_start, in_end);
   }
 
   void assign(std::initializer_list<T> IL) {
     clear();
     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);
 
     assert(this->isReferenceToStorage(CI) && "Iterator to erase is out of bounds.");
 
     iterator N = I;
     // Shift all elts down one.

llvm/lib/Support/FileCollector.cpp

void FileCollector::addFileImpl(StringRef SrcPath) {
   // We need an absolute src path to append to the root.
   SmallString<256> AbsoluteSrc = SrcPath;
   sys::fs::make_absolute(AbsoluteSrc);
 
   // Canonicalize src to a native path to avoid mixed separator styles.
   sys::path::native(AbsoluteSrc);
 
   // Remove redundant leading "./" pieces and consecutive separators.
-  AbsoluteSrc = sys::path::remove_leading_dotslash(AbsoluteSrc);
+  {
+    StringRef DroppedDotSlash = sys::path::remove_leading_dotslash(AbsoluteSrc);
+    if (DroppedDotSlash.begin() != AbsoluteSrc.begin())
+      AbsoluteSrc.erase(AbsoluteSrc.begin(), DroppedDotSlash.begin());
+  }
 
   // Canonicalize the source path by removing "..", "." components.
   SmallString<256> VirtualPath = AbsoluteSrc;
   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.

llvm/unittests/ADT/SmallVectorTest.cpp

 TYPED_TEST(SmallVectorTest, AppendRepeatedNonForwardIterator) {
   SCOPED_TRACE("AppendRepeatedTest");
 
   this->theVector.push_back(Constructable(1));
   this->theVector.append(output_iterator(), output_iterator());
   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");
 
   this->theVector.push_back(Constructable(1));
   this->theVector.assign(2, Constructable(77));
   this->assertValuesInOrder(this->theVector, 2u, 77, 77);
 }

 TYPED_TEST(SmallVectorTest, AssignNonIterTest) {
   SCOPED_TRACE("AssignTest");
 
   this->theVector.push_back(Constructable(1));
   this->theVector.assign(2, 7);
   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");
 
   // Set up our vector with a single element, but enough capacity for 4.
   this->theVector.reserve(4);
   this->theVector.push_back(Constructable(1));