[llvm] r214737 - Path: Stop claiming path::const_iterator is bidirectional

Duncan P. N. Exon Smith dexonsmith at apple.com
Mon Aug 4 11:13:21 PDT 2014


> On 2014-Aug-04, at 10:36, Justin Bogner <mail at justinbogner.com> wrote:
> 
> Author: bogner
> Date: Mon Aug  4 12:36:41 2014
> New Revision: 214737
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=214737&view=rev
> Log:
> 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,

Is this really just an `input_iterator`?  I would have expected it to
be a `forward_iterator` at least.

Also, can these take advantage of `iterator_facade_base`?

> 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.
> 
> Modified:
>    llvm/trunk/include/llvm/Support/Path.h
>    llvm/trunk/lib/Support/Path.cpp
>    llvm/trunk/unittests/Support/Path.cpp
> 
> Modified: llvm/trunk/include/llvm/Support/Path.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/Path.h?rev=214737&r1=214736&r2=214737&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/Support/Path.h (original)
> +++ llvm/trunk/include/llvm/Support/Path.h Mon Aug  4 12:36:41 2014
> @@ -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<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) {
> -  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
> 
> Modified: llvm/trunk/lib/Support/Path.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/Path.cpp?rev=214737&r1=214736&r2=214737&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Support/Path.cpp (original)
> +++ llvm/trunk/lib/Support/Path.cpp Mon Aug  4 12:36:41 2014
> @@ -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<char> &path)
> }
> 
> const StringRef filename(StringRef path) {
> -  return *(--end(path));
> +  return *rbegin(path);
> }
> 
> const StringRef stem(StringRef path) {
> 
> Modified: llvm/trunk/unittests/Support/Path.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/Path.cpp?rev=214737&r1=214736&r2=214737&view=diff
> ==============================================================================
> --- llvm/trunk/unittests/Support/Path.cpp (original)
> +++ llvm/trunk/unittests/Support/Path.cpp Mon Aug  4 12:36:41 2014
> @@ -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]));
>   }
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits




More information about the llvm-commits mailing list