[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