[llvm] 1b651be - ADT: Fix const-correctness of iterator adaptors
Duncan P. N. Exon Smith via llvm-commits
llvm-commits at lists.llvm.org
Fri Nov 12 11:25:02 PST 2021
Author: Duncan P. N. Exon Smith
Date: 2021-11-12T11:24:17-08:00
New Revision: 1b651be0465de70cfa22ce4f715d3501a4dcffc1
URL: https://github.com/llvm/llvm-project/commit/1b651be0465de70cfa22ce4f715d3501a4dcffc1
DIFF: https://github.com/llvm/llvm-project/commit/1b651be0465de70cfa22ce4f715d3501a4dcffc1.diff
LOG: ADT: Fix const-correctness of iterator adaptors
This fixes const-correctness of iterator adaptors, dropping non-`const`
overloads for `operator*()`.
Iterators, like the pointers that they generalize, have two types of
`const`.
The `const` qualifier on members indicates whether the iterator itself
can be changed. This is analagous to `int *const`.
The `const` qualifier on return values of `operator*()`, `operator[]()`,
and `operator->()` controls whether the the pointed-to value can be
changed. This is analogous to `const int *`.
Since `operator*()` does not (in principle) change the iterator, then
there should only be one definition, which is `const`-qualified. E.g.,
iterators wrapping `int*` should look like:
```
int *operator*() const; // always const-qualified, no overloads
```
ba7a6b314fd14bb2c9ff5d3f4fe2b6525514cada changed `iterator_adaptor_base`
away from this to work around bugs in other iterator adaptors. That was
already reverted. This patch adds back its test, which combined
llvm::enumerate() and llvm::make_filter_range(), adds a test for
iterator_adaptor_base itself, and cleans up the `const`-ness of the
other iterator adaptors.
This also updates the documented requirements for
`iterator_facade_base`:
```
/// OLD:
/// - const T &operator*() const;
/// - T &operator*();
/// New:
/// - T &operator*() const;
```
In a future commit we might also clean up `iterator_facade`'s overloads
of `operator->()` and `operator[]()`. These already (correctly) return
non-`const` proxies regardless of the iterator's `const` qualifier.
Differential Revision: https://reviews.llvm.org/D113158
Added:
Modified:
llvm/include/llvm/ADT/STLExtras.h
llvm/include/llvm/ADT/iterator.h
llvm/unittests/ADT/IteratorTest.cpp
Removed:
################################################################################
diff --git a/llvm/include/llvm/ADT/STLExtras.h b/llvm/include/llvm/ADT/STLExtras.h
index 6c2fb1029a918..d3a9ce8e76a05 100644
--- a/llvm/include/llvm/ADT/STLExtras.h
+++ b/llvm/include/llvm/ADT/STLExtras.h
@@ -671,9 +671,7 @@ struct zip_common : public zip_traits<ZipType, Iters...> {
public:
zip_common(Iters &&... ts) : iterators(std::forward<Iters>(ts)...) {}
- value_type operator*() { return deref(std::index_sequence_for<Iters...>{}); }
-
- const value_type operator*() const {
+ value_type operator*() const {
return deref(std::index_sequence_for<Iters...>{});
}
@@ -844,8 +842,6 @@ class zip_longest_iterator
: iterators(std::forward<Iters>(ts.first)...),
end_iterators(std::forward<Iters>(ts.second)...) {}
- value_type operator*() { return deref(std::index_sequence_for<Iters...>{}); }
-
value_type operator*() const {
return deref(std::index_sequence_for<Iters...>{});
}
@@ -1939,8 +1935,7 @@ template <typename R> struct result_pair {
}
std::size_t index() const { return Index; }
- const value_reference value() const { return *Iter; }
- value_reference value() { return *Iter; }
+ value_reference value() const { return *Iter; }
private:
std::size_t Index = std::numeric_limits<std::size_t>::max();
@@ -1949,11 +1944,8 @@ template <typename R> struct result_pair {
template <typename R>
class enumerator_iter
- : public iterator_facade_base<
- enumerator_iter<R>, std::forward_iterator_tag, result_pair<R>,
- typename std::iterator_traits<IterOfRange<R>>::
diff erence_type,
- typename std::iterator_traits<IterOfRange<R>>::pointer,
- typename std::iterator_traits<IterOfRange<R>>::reference> {
+ : public iterator_facade_base<enumerator_iter<R>, std::forward_iterator_tag,
+ const result_pair<R>> {
using result_type = result_pair<R>;
public:
@@ -1963,7 +1955,6 @@ class enumerator_iter
enumerator_iter(std::size_t Index, IterOfRange<R> Iter)
: Result(Index, Iter) {}
- result_type &operator*() { return Result; }
const result_type &operator*() const { return Result; }
enumerator_iter &operator++() {
diff --git a/llvm/include/llvm/ADT/iterator.h b/llvm/include/llvm/ADT/iterator.h
index b9bac9d385c0b..070320581a71c 100644
--- a/llvm/include/llvm/ADT/iterator.h
+++ b/llvm/include/llvm/ADT/iterator.h
@@ -42,8 +42,7 @@ namespace llvm {
/// (All of the following methods)
/// - DerivedT &operator=(const DerivedT &R);
/// - bool operator==(const DerivedT &R) const;
-/// - const T &operator*() const;
-/// - T &operator*();
+/// - T &operator*() const;
/// - DerivedT &operator++();
///
/// Bidirectional Iterators:
@@ -348,8 +347,7 @@ class pointer_iterator
explicit pointer_iterator(WrappedIteratorT u)
: pointer_iterator::iterator_adaptor_base(std::move(u)) {}
- T &operator*() { return Ptr = &*this->I; }
- const T &operator*() const { return Ptr = &*this->I; }
+ T &operator*() const { return Ptr = &*this->I; }
};
template <typename RangeT, typename WrappedIteratorT =
diff --git a/llvm/unittests/ADT/IteratorTest.cpp b/llvm/unittests/ADT/IteratorTest.cpp
index de74811321a9a..b443afc0fc696 100644
--- a/llvm/unittests/ADT/IteratorTest.cpp
+++ b/llvm/unittests/ADT/IteratorTest.cpp
@@ -52,6 +52,90 @@ using IsAdaptedIterCategorySame =
std::is_same<typename std::iterator_traits<It>::iterator_category,
typename std::iterator_traits<A<It>>::iterator_category>;
+// Check that dereferencing works correctly adapting pointers and proxies.
+template <class T>
+struct PointerWrapper : public iterator_adaptor_base<PointerWrapper<T>, T *> {
+ PointerWrapper(T *I) : iterator_adaptor_base<PointerWrapper, T *>(I) {}
+};
+struct IntProxy {
+ int &I;
+ IntProxy(int &I) : I(I) {}
+ void operator=(int NewValue) { I = NewValue; }
+};
+struct ConstIntProxy {
+ const int &I;
+ ConstIntProxy(const int &I) : I(I) {}
+};
+template <class T, class ProxyT>
+struct PointerProxyWrapper
+ : public iterator_adaptor_base<PointerProxyWrapper<T, ProxyT>, T *,
+ std::random_access_iterator_tag, T,
+ ptr
diff _t, T *, ProxyT> {
+ PointerProxyWrapper(T *I)
+ : iterator_adaptor_base<PointerProxyWrapper, T *,
+ std::random_access_iterator_tag, T, ptr
diff _t,
+ T *, ProxyT>(I) {}
+};
+using IntIterator = PointerWrapper<int>;
+using ConstIntIterator = PointerWrapper<const int>;
+using IntProxyIterator = PointerProxyWrapper<int, IntProxy>;
+using ConstIntProxyIterator = PointerProxyWrapper<const int, ConstIntProxy>;
+
+template <class T,
+ std::enable_if_t<std::is_assignable<T, int>::value, bool> = false>
+constexpr bool canAssignFromInt(T &&) {
+ return true;
+}
+template <class T,
+ std::enable_if_t<!std::is_assignable<T, int>::value, bool> = false>
+constexpr bool canAssignFromInt(T &&) {
+ return false;
+}
+
+TEST(IteratorAdaptorTest, Dereference) {
+ int Number = 1;
+
+ // Construct some iterators and check whether they can be assigned to.
+ IntIterator I(&Number);
+ const IntIterator IC(&Number);
+ ConstIntIterator CI(&Number);
+ const ConstIntIterator CIC(&Number);
+ EXPECT_EQ(true, canAssignFromInt(*I)); // int *
+ EXPECT_EQ(true, canAssignFromInt(*IC)); // int *const
+ EXPECT_EQ(false, canAssignFromInt(*CI)); // const int *
+ EXPECT_EQ(false, canAssignFromInt(*CIC)); // const int *const
+
+ // Prove that dereference and assignment work.
+ EXPECT_EQ(1, *I);
+ EXPECT_EQ(1, *IC);
+ EXPECT_EQ(1, *CI);
+ EXPECT_EQ(1, *CIC);
+ *I = 2;
+ EXPECT_EQ(2, Number);
+ *IC = 3;
+ EXPECT_EQ(3, Number);
+
+ // Construct some proxy iterators and check whether they can be assigned to.
+ IntProxyIterator P(&Number);
+ const IntProxyIterator PC(&Number);
+ ConstIntProxyIterator CP(&Number);
+ const ConstIntProxyIterator CPC(&Number);
+ EXPECT_EQ(true, canAssignFromInt(*P)); // int *
+ EXPECT_EQ(true, canAssignFromInt(*PC)); // int *const
+ EXPECT_EQ(false, canAssignFromInt(*CP)); // const int *
+ EXPECT_EQ(false, canAssignFromInt(*CPC)); // const int *const
+
+ // Prove that dereference and assignment work.
+ EXPECT_EQ(3, (*P).I);
+ EXPECT_EQ(3, (*PC).I);
+ EXPECT_EQ(3, (*CP).I);
+ EXPECT_EQ(3, (*CPC).I);
+ *P = 4;
+ EXPECT_EQ(4, Number);
+ *PC = 5;
+ EXPECT_EQ(5, Number);
+}
+
// pointeE_iterator
static_assert(IsAdaptedIterCategorySame<pointee_iterator_defaulted,
RandomAccessIter>::value, "");
@@ -178,6 +262,16 @@ TEST(FilterIteratorTest, Lambda) {
EXPECT_EQ((SmallVector<int, 3>{1, 3, 5}), Actual);
}
+TEST(FilterIteratorTest, Enumerate) {
+ auto IsOdd = [](auto N) { return N.value() % 2 == 1; };
+ int A[] = {0, 1, 2, 3, 4, 5, 6};
+ auto Enumerate = llvm::enumerate(A);
+ SmallVector<int> Actual;
+ for (auto IndexedValue : make_filter_range(Enumerate, IsOdd))
+ Actual.push_back(IndexedValue.value());
+ EXPECT_EQ((SmallVector<int, 3>{1, 3, 5}), Actual);
+}
+
TEST(FilterIteratorTest, CallableObject) {
int Counter = 0;
struct Callable {
More information about the llvm-commits
mailing list