From 981eb59138eea543af5fc628399480eb11d27016 Mon Sep 17 00:00:00 2001 From: Justin Bogner Date: Mon, 4 Aug 2014 17:36:41 +0000 Subject: [PATCH] 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 | 50 ++++++++++++++++++++++--------------- lib/Support/Path.cpp | 39 ++++++++++++++++++++--------- unittests/Support/Path.cpp | 6 ++--- 3 files changed, 60 insertions(+), 35 deletions(-) diff --git a/include/llvm/Support/Path.h b/include/llvm/Support/Path.h index cf821f0ef4a..7206df4786d 100644 --- a/include/llvm/Support/Path.h +++ b/include/llvm/Support/Path.h @@ -30,13 +30,13 @@ namespace path { /// @brief Path iterator. /// -/// This is a bidirectional iterator that iterates over the individual -/// components in \a path. The forward traversal order is as follows: +/// This is an input iterator that iterates over the individual components in +/// \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 { 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 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 { + 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) { - return reverse_iterator(end(path)); -} +reverse_iterator rbegin(StringRef 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) { - return reverse_iterator(begin(path)); -} +reverse_iterator rend(StringRef path); /// @} /// @name Lexical Modifiers diff --git a/lib/Support/Path.cpp b/lib/Support/Path.cpp index d5a0ec55c68..2dd6741c735 100644 --- a/lib/Support/Path.cpp +++ b/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 { - return Path.begin() == RHS.Path.begin() && +bool reverse_iterator::operator==(const reverse_iterator &RHS) const { + 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 &path) { } const StringRef filename(StringRef path) { - return *(--end(path)); + return *rbegin(path); } const StringRef stem(StringRef path) { diff --git a/unittests/Support/Path.cpp b/unittests/Support/Path.cpp index db85347e921..bb1428f4de4 100644 --- a/unittests/Support/Path.cpp +++ b/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])); }