[llvm] r214737 - Path: Stop claiming path::const_iterator is bidirectional
Justin Bogner
mail at justinbogner.com
Mon Aug 4 10:36:42 PDT 2014
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,
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]));
}
More information about the llvm-commits
mailing list