Path: Stop claiming path::const_iterator is bidirectional

path::const_iterator claims that it's a bidirectional iterator, but it
doesn't satisfy all of the contracts for a bidirectional iterator.
For example, n3376 24.2.5 p6 says "If a and b are both dereferenceable,
then a == b if and only if *a and *b are bound to the same object",
but this doesn't work with how we stash and recreate Components.

This means that our use of reverse_iterator on this type is invalid
and leads to many of the valgrind errors we're hitting, as explained
by Tilmann Scheller here:

    http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20140728/228654.html

Instead, we admit that path::const_iterator is only an input_iterator,
and implement a second input_iterator for path::reverse_iterator (by
changing const_iterator::operator-- to reverse_iterator::operator++).
All of the uses of this just traverse once over the path in one
direction or the other anyway.

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@214737 91177308-0d34-0410-b5e6-96231b3b80d8

include/llvm/Support/Path.h

@@ -30,13 +30,13 @@ namespace path {
 
 /// @brief Path iterator.
 ///
-/// This is a bidirectional iterator that iterates over the individual
+/// This is an input iterator that iterates over the individual components in
-/// components in \a path. The forward traversal order is as follows:
+/// \a path. The traversal order is as follows:
 /// * The root-name element, if present.
 /// * The root-directory element, if present.
 /// * Each successive filename element, if present.
 /// * Dot, if one or more trailing non-root slash characters are present.
-/// The backwards traversal order is the reverse of forward traversal.
+/// Traversing backwards is possible with \a reverse_iterator
 ///
 /// Iteration examples. Each component is separated by ',':
 /// @code
@@ -47,7 +47,8 @@ namespace path {
 ///   ../        => ..,.
 ///   C:\foo\bar => C:,/,foo,bar
 /// @endcode
-class const_iterator {
+class const_iterator
+    : public std::iterator<std::input_iterator_tag, const StringRef> {
   StringRef Path;      ///< The entire path.
   StringRef Component; ///< The current component. Not necessarily in Path.
   size_t    Position;  ///< The iterators current position within Path.
@@ -57,26 +58,39 @@ class const_iterator {
   friend const_iterator end(StringRef path);
 
 public:
-  typedef const StringRef value_type;
-  typedef ptrdiff_t difference_type;
-  typedef value_type &reference;
-  typedef value_type *pointer;
-  typedef std::bidirectional_iterator_tag iterator_category;
-
   reference operator*() const { return Component; }
   pointer   operator->() const { return &Component; }
   const_iterator &operator++();    // preincrement
   const_iterator &operator++(int); // postincrement
-  const_iterator &operator--();    // predecrement
-  const_iterator &operator--(int); // postdecrement
   bool operator==(const const_iterator &RHS) const;
-  bool operator!=(const const_iterator &RHS) const;
+  bool operator!=(const const_iterator &RHS) const { return !(*this == RHS); }
 
   /// @brief Difference in bytes between this and RHS.
   ptrdiff_t operator-(const const_iterator &RHS) const;
 };
 
-typedef std::reverse_iterator<const_iterator> reverse_iterator;
+/// @brief Reverse path iterator.
+///
+/// This is an input iterator that iterates over the individual components in
+/// \a path in reverse order. The traversal order is exactly reversed from that
+/// of \a const_iterator
+class reverse_iterator
+    : public std::iterator<std::input_iterator_tag, const StringRef> {
+  StringRef Path;      ///< The entire path.
+  StringRef Component; ///< The current component. Not necessarily in Path.
+  size_t    Position;  ///< The iterators current position within Path.
+
+  friend reverse_iterator rbegin(StringRef path);
+  friend reverse_iterator rend(StringRef path);
+
+public:
+  reference operator*() const { return Component; }
+  pointer   operator->() const { return &Component; }
+  reverse_iterator &operator++();    // preincrement
+  reverse_iterator &operator++(int); // postincrement
+  bool operator==(const reverse_iterator &RHS) const;
+  bool operator!=(const reverse_iterator &RHS) const { return !(*this == RHS); }
+};
 
 /// @brief Get begin iterator over \a path.
 /// @param path Input path.
@@ -91,16 +105,12 @@ const_iterator end(StringRef path);
 /// @brief Get reverse begin iterator over \a path.
 /// @param path Input path.
 /// @returns Iterator initialized with the first reverse component of \a path.
-inline reverse_iterator rbegin(StringRef path) {
+reverse_iterator rbegin(StringRef path);
-  return reverse_iterator(end(path));
-}
 
 /// @brief Get reverse end iterator over \a path.
 /// @param path Input path.
 /// @returns Iterator initialized to the reverse end of \a path.
-inline reverse_iterator rend(StringRef path) {
+reverse_iterator rend(StringRef path);
-  return reverse_iterator(begin(path));
-}
 
 /// @}
 /// @name Lexical Modifiers

lib/Support/Path.cpp

@@ -308,7 +308,30 @@ const_iterator &const_iterator::operator++() {
   return *this;
 }
 
-const_iterator &const_iterator::operator--() {
+bool const_iterator::operator==(const const_iterator &RHS) const {
+  return Path.begin() == RHS.Path.begin() && Position == RHS.Position;
+}
+
+ptrdiff_t const_iterator::operator-(const const_iterator &RHS) const {
+  return Position - RHS.Position;
+}
+
+reverse_iterator rbegin(StringRef Path) {
+  reverse_iterator I;
+  I.Path = Path;
+  I.Position = Path.size();
+  return ++I;
+}
+
+reverse_iterator rend(StringRef Path) {
+  reverse_iterator I;
+  I.Path = Path;
+  I.Component = Path.substr(0, 0);
+  I.Position = 0;
+  return I;
+}
+
+reverse_iterator &reverse_iterator::operator++() {
   // If we're at the end and the previous char was a '/', return '.' unless
   // we are the root path.
   size_t root_dir_pos = root_dir_start(Path);
@@ -335,19 +358,11 @@ const_iterator &const_iterator::operator--() {
   return *this;
 }
 
-bool const_iterator::operator==(const const_iterator &RHS) const {
+bool reverse_iterator::operator==(const reverse_iterator &RHS) const {
-  return Path.begin() == RHS.Path.begin() &&
+  return Path.begin() == RHS.Path.begin() && Component == RHS.Component &&
          Position == RHS.Position;
 }
 
-bool const_iterator::operator!=(const const_iterator &RHS) const {
-  return !(*this == RHS);
-}
-
-ptrdiff_t const_iterator::operator-(const const_iterator &RHS) const {
-  return Position - RHS.Position;
-}
-
 const StringRef root_path(StringRef path) {
   const_iterator b = begin(path),
                  pos = b,
@@ -532,7 +547,7 @@ void native(SmallVectorImpl<char> &path) {
 }
 
 const StringRef filename(StringRef path) {
-  return *(--end(path));
+  return *rbegin(path);
 }
 
 const StringRef stem(StringRef path) {

unittests/Support/Path.cpp

@@ -141,7 +141,7 @@ TEST(Support, Path) {
     StringRef filename(temp_store.begin(), temp_store.size()), stem, ext;
     stem = path::stem(filename);
     ext  = path::extension(filename);
-    EXPECT_EQ(*(--sys::path::end(filename)), (stem + ext).str());
+    EXPECT_EQ(*sys::path::rbegin(filename), (stem + ext).str());
 
     path::native(*i, temp_store);
   }
@@ -227,7 +227,7 @@ TEST(Support, AbsolutePathIteratorEnd) {
 #endif
 
   for (StringRef Path : Paths) {
-    StringRef LastComponent = *--path::end(Path);
+    StringRef LastComponent = *path::rbegin(Path);
     EXPECT_EQ(".", LastComponent);
   }
 
@@ -239,7 +239,7 @@ TEST(Support, AbsolutePathIteratorEnd) {
 #endif
 
   for (StringRef Path : RootPaths) {
-    StringRef LastComponent = *--path::end(Path);
+    StringRef LastComponent = *path::rbegin(Path);
     EXPECT_EQ(1u, LastComponent.size());
     EXPECT_TRUE(path::is_separator(LastComponent[0]));
   }