[libc][cpp] reverse_iterator support #85702

nickdesaulniers · 2024-03-18T21:17:26Z

Towards the goal of implementing __cxa_finalize (#85651) I'd like to be able to
reverse iterate over cpp::arrays such as the one used in FixedVector.

Implement the enough iterator support to be able to iterate a cpp::array in
reverse, and add tests.

Of note, reverse iterator's begin() refers to forward iterator's end() (and
vice versa). When dereferenced (operator*), the reverse iterator returns a copy
that's been pre-decremented (the underlying forward iterator is advanced).

Towards the goal of implementing __cxa_finalize (llvm#85651) I'd like to be able to reverse iterator over cpp::arrays such as the one used in FixedVector. Implement the enough iterator support to be able to iterate a cpp::array in reverse, and add tests. Of note, reverse iterator's begin() refers to forward iterator's end() (and vice versa). When dereferenced (operator*), the reverse iterator returns a copy that's been pre-decremented (the underlying forward iterator is advanced).

llvmbot · 2024-03-18T21:18:08Z

@llvm/pr-subscribers-libc

Author: Nick Desaulniers (nickdesaulniers)

Changes

Towards the goal of implementing __cxa_finalize (#85651) I'd like to be able to
reverse iterator over cpp::arrays such as the one used in FixedVector.

Implement the enough iterator support to be able to iterate a cpp::array in
reverse, and add tests.

Of note, reverse iterator's begin() refers to forward iterator's end() (and
vice versa). When dereferenced (operator*), the reverse iterator returns a copy
that's been pre-decremented (the underlying forward iterator is advanced).

Full diff: https://github.com/llvm/llvm-project/pull/85702.diff

4 Files Affected:

(modified) libc/src/__support/CPP/array.h (+23)
(added) libc/src/__support/CPP/iterator.h (+62)
(modified) libc/test/src/__support/CPP/CMakeLists.txt (+10)
(added) libc/test/src/__support/CPP/array_test.cpp (+66)

diff --git a/libc/src/__support/CPP/array.h b/libc/src/__support/CPP/array.h
index fb5a79225beb7d..4e69ba003e800b 100644
--- a/libc/src/__support/CPP/array.h
+++ b/libc/src/__support/CPP/array.h
@@ -9,6 +9,7 @@
 #ifndef LLVM_LIBC_SRC___SUPPORT_CPP_ARRAY_H
 #define LLVM_LIBC_SRC___SUPPORT_CPP_ARRAY_H
 
+#include "src/__support/CPP/iterator.h" // reverse_iterator
 #include "src/__support/macros/attributes.h"
 #include <stddef.h> // For size_t.
 
@@ -23,6 +24,8 @@ template <class T, size_t N> struct array {
   using value_type = T;
   using iterator = T *;
   using const_iterator = const T *;
+  using reverse_iterator = cpp::reverse_iterator<iterator>;
+  using const_reverse_iterator = cpp::reverse_iterator<const_iterator>;
 
   LIBC_INLINE constexpr T *data() { return Data; }
   LIBC_INLINE constexpr const T *data() const { return Data; }
@@ -45,9 +48,29 @@ template <class T, size_t N> struct array {
 
   LIBC_INLINE constexpr iterator begin() { return Data; }
   LIBC_INLINE constexpr const_iterator begin() const { return Data; }
+  LIBC_INLINE constexpr const_iterator cbegin() const { return begin(); }
 
   LIBC_INLINE constexpr iterator end() { return Data + N; }
   LIBC_INLINE constexpr const_iterator end() const { return Data + N; }
+  LIBC_INLINE constexpr const_iterator cend() const { return end(); }
+
+  LIBC_INLINE constexpr reverse_iterator rbegin() {
+    return reverse_iterator{end()};
+  }
+  LIBC_INLINE constexpr const_reverse_iterator rbegin() const {
+    return const_reverse_iterator{end()};
+  }
+  LIBC_INLINE constexpr const_reverse_iterator crbegin() const {
+    return rbegin();
+  }
+
+  LIBC_INLINE constexpr reverse_iterator rend() {
+    return reverse_iterator{begin()};
+  }
+  LIBC_INLINE constexpr const_reverse_iterator rend() const {
+    return const_reverse_iterator{begin()};
+  }
+  LIBC_INLINE constexpr const_reverse_iterator crend() const { return rend(); }
 };
 
 } // namespace cpp
diff --git a/libc/src/__support/CPP/iterator.h b/libc/src/__support/CPP/iterator.h
new file mode 100644
index 00000000000000..1a52f096e80005
--- /dev/null
+++ b/libc/src/__support/CPP/iterator.h
@@ -0,0 +1,62 @@
+//===-- Standalone implementation of iterator -------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_SRC___SUPPORT_CPP_ITERATOR_H
+#define LLVM_LIBC_SRC___SUPPORT_CPP_ITERATOR_H
+
+#include "src/__support/CPP/type_traits/enable_if.h"
+#include "src/__support/CPP/type_traits/is_convertible.h"
+#include "src/__support/CPP/type_traits/is_same.h"
+#include "src/__support/macros/attributes.h"
+
+namespace LIBC_NAMESPACE {
+namespace cpp {
+
+template <typename T> struct iterator_traits;
+template <typename T> struct iterator_traits<T *> {
+  using reference = T &;
+};
+
+template <typename Iter> class reverse_iterator {
+  Iter current;
+
+public:
+  using reference = typename iterator_traits<Iter>::reference;
+
+  reverse_iterator() : current() {}
+  constexpr explicit reverse_iterator(Iter it) : current(it) {}
+
+  template <typename Other,
+            cpp::enable_if_t<!cpp::is_same_v<Iter, Other> &&
+                                 cpp::is_convertible_v<const Other &, Iter>,
+                             int> = 0>
+  constexpr explicit reverse_iterator(const Other &it) : current(it) {}
+
+  constexpr reference operator*() const {
+    Iter tmp = current;
+    return *--tmp;
+  }
+  constexpr reverse_iterator operator--() {
+    ++current;
+    return *this;
+  }
+  constexpr reverse_iterator &operator++() {
+    --current;
+    return *this;
+  }
+  constexpr reverse_iterator operator++(int) {
+    reverse_iterator tmp(*this);
+    --current;
+    return tmp;
+  }
+};
+
+} // namespace cpp
+} // namespace LIBC_NAMESPACE
+
+#endif // LLVM_LIBC_SRC___SUPPORT_CPP_ITERATOR_H
diff --git a/libc/test/src/__support/CPP/CMakeLists.txt b/libc/test/src/__support/CPP/CMakeLists.txt
index f94429e03b3cbc..74aa0c705ec467 100644
--- a/libc/test/src/__support/CPP/CMakeLists.txt
+++ b/libc/test/src/__support/CPP/CMakeLists.txt
@@ -1,5 +1,15 @@
 add_custom_target(libc-cpp-utils-tests)
 
+add_libc_test(
+  array_test
+  SUITE
+    libc-cpp-utils-tests
+  SRCS
+    array_test.cpp
+  DEPENDS
+    libc.src.__support.CPP.array
+  )
+
 add_libc_test(
   bit_test
   SUITE
diff --git a/libc/test/src/__support/CPP/array_test.cpp b/libc/test/src/__support/CPP/array_test.cpp
new file mode 100644
index 00000000000000..f2d7bff636e42c
--- /dev/null
+++ b/libc/test/src/__support/CPP/array_test.cpp
@@ -0,0 +1,66 @@
+//===-- Unittests for Array -----------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "src/__support/CPP/array.h"
+#include "test/UnitTest/Test.h"
+
+using LIBC_NAMESPACE::cpp::array;
+
+TEST(LlvmLibcArrayTest, Basic) {
+  array<int, 3> a = {0, 1, 2};
+
+  ASSERT_EQ(a.data(), &a.front());
+  ASSERT_EQ(a.front(), 0);
+  ASSERT_EQ(a.back(), 2);
+  ASSERT_EQ(a.size(), size_t{3});
+  ASSERT_EQ(a[1], 1);
+  ASSERT_FALSE(a.empty());
+  ASSERT_NE(a.begin(), a.end());
+  ASSERT_EQ(*a.begin(), a.front());
+
+  auto it = a.rbegin();
+  ASSERT_EQ(*it, 2);
+  ASSERT_EQ(*(++it), 1);
+  ASSERT_EQ(*(++it), 0);
+
+  for (int &x : a)
+    ASSERT_GE(x, 0);
+}
+
+// Test const_iterator and const variant methods.
+TEST(LlvmLibcArrayTest, Const) {
+  const array<int, 3> z = {3, 4, 5};
+
+  ASSERT_EQ(3, z.front());
+  ASSERT_EQ(4, z[1]);
+  ASSERT_EQ(5, z.back());
+  ASSERT_EQ(3, *z.data());
+
+  // begin, cbegin, end, cend
+  array<int, 3>::const_iterator it2 = z.begin();
+  ASSERT_EQ(*it2, z.front());
+  it2 = z.cbegin();
+  ASSERT_EQ(*it2, z.front());
+  it2 = z.end();
+  ASSERT_NE(it2, z.begin());
+  it2 = z.cend();
+  ASSERT_NE(it2, z.begin());
+
+  // rbegin, crbegin, rend, crend
+  array<int, 3>::const_reverse_iterator it = z.rbegin();
+  ASSERT_EQ(*it, z.back());
+  it = z.crbegin();
+  ASSERT_EQ(*it, z.back());
+  it = z.rend();
+  ASSERT_EQ(*--it, z.front());
+  it = z.crend();
+  ASSERT_EQ(*--it, z.front());
+
+  for (const int &x : z)
+    ASSERT_GE(x, 0);
+}

gchatelet · 2024-03-19T10:28:31Z

libc/src/__support/CPP/iterator.h

+template <typename Iter> class reverse_iterator {
+  Iter current;
+
+public:
+  using reference = typename iterator_traits<Iter>::reference;
+
+  reverse_iterator() : current() {}
+  constexpr explicit reverse_iterator(Iter it) : current(it) {}
+
+  template <typename Other,
+            cpp::enable_if_t<!cpp::is_same_v<Iter, Other> &&
+                                 cpp::is_convertible_v<const Other &, Iter>,
+                             int> = 0>
+  constexpr explicit reverse_iterator(const Other &it) : current(it) {}
+
+  constexpr reference operator*() const {
+    Iter tmp = current;
+    return *--tmp;
+  }
+  constexpr reverse_iterator operator--() {
+    ++current;
+    return *this;
+  }
+  constexpr reverse_iterator &operator++() {
+    --current;
+    return *this;
+  }
+  constexpr reverse_iterator operator++(int) {
+    reverse_iterator tmp(*this);
+    --current;
+    return tmp;
+  }
+};


LIBC_INLINE everywhere

done in 965130c

gchatelet · 2024-03-19T10:33:34Z

libc/src/__support/CPP/iterator.h

+                             int> = 0>
+  constexpr explicit reverse_iterator(const Other &it) : current(it) {}
+
+  constexpr reference operator*() const {


[[nodiscard]] ?

https://en.cppreference.com/w/cpp/iterator/reverse_iterator/operator* doesn't mention nodiscard. Should I check the standard?

https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/n4640.pdf S24.5.1.1 "Class template reverse_iterator" has an example. It's not using [[nodiscard]]. Same for https://isocpp.org/files/papers/N4860.pdf (S23.5.1.1).

gchatelet · 2024-03-19T10:34:46Z

libc/src/__support/CPP/array.h

+  LIBC_INLINE constexpr const_reverse_iterator rend() const {
+    return const_reverse_iterator{begin()};
+  }
+  LIBC_INLINE constexpr const_reverse_iterator crend() const { return rend(); }


Most functions here should be [[nodiscard]] I believe.

https://en.cppreference.com/w/cpp/container/array/rend also doesn't mention nodiscard.

I guess I'm happy to add it, regardless, since nodiscard should have been the language default IMO.

C++17 S23.3.7.1 Class template array overview also doesn't mention [[nodiscard]] on these methods.

Hmm...now I'm on the fence. I'm going to land these without, but I would not be opposed to adding them in a future PR.

Coincidentally, I think some WG21 are in Tokyo right now, and this came up for discussion. @lntue pointed me to a discussion in #libcxx channel of llvm discord where Mark de Wever said:

Going forward it seems LEWG is happy to leave nodiscard to implementors 🙂 @philnik regarding your nodiscard ideas, did you ask MSVC STL what they do? @ldionne can you put nodiscard on the agenda for our next meeting?

Hehe! I didn't mean to confuse you with [[nodiscard]] but I'm amazed by the outcome!

Tangentially I think we're missing the following functions for reverse_iterator to be useful.

LIBC_INLINE friend constexpr bool operator==(const reverse_iterator &lhs, const reverse_iterator &rhs) { return lhs.current == rhs.current; } LIBC_INLINE friend constexpr bool operator!=(const reverse_iterator &lhs, const reverse_iterator &rhs) { return lhs.current != rhs.current; }

as in for (auto itr = value.rbegin(); itr **!=** value.rend(); ++itr).

Patch llvm#85702 added a new iterator header for array to use, but didn't add it as a dependency in the bazel build. This patch fixes the issue.

michaelrj-google · 2024-03-19T18:28:50Z

This patch broke the bazel build. Patch to fix it here: #85840

Patch #85702 added a new iterator header for array to use, but didn't add it as a dependency in the bazel build. This patch fixes the issue.

Towards the goal of implementing __cxa_finalize (llvm#85651) I'd like to be able to reverse iterate over cpp::arrays such as the one used in FixedVector. Implement the enough iterator support to be able to iterate a cpp::array in reverse, and add tests. Of note, reverse iterator's begin() refers to forward iterator's end() (and vice versa). When dereferenced (operator*), the reverse iterator returns a copy that's been pre-decremented (the underlying forward iterator is advanced).

Patch llvm#85702 added a new iterator header for array to use, but didn't add it as a dependency in the bazel build. This patch fixes the issue.

llvmbot added the libc label Mar 18, 2024

nickdesaulniers requested a review from gchatelet March 18, 2024 21:19

gchatelet approved these changes Mar 19, 2024

View reviewed changes

LIBC_INLINE

965130c

nickdesaulniers merged commit c63a291 into llvm:main Mar 19, 2024

nickdesaulniers deleted the array branch March 19, 2024 16:26

michaelrj-google mentioned this pull request Mar 19, 2024

[libc][bazel] fix missing iterator inclusion #85840

Merged

michaelrj-google added a commit that referenced this pull request Mar 19, 2024

[libc][bazel] fix missing iterator inclusion (#85840)

1d296ec

Patch #85702 added a new iterator header for array to use, but didn't add it as a dependency in the bazel build. This patch fixes the issue.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[libc][cpp] reverse_iterator support #85702

[libc][cpp] reverse_iterator support #85702

Uh oh!

nickdesaulniers commented Mar 18, 2024 •

edited

Loading

Uh oh!

llvmbot commented Mar 18, 2024

Uh oh!

gchatelet Mar 19, 2024

Uh oh!

nickdesaulniers Mar 19, 2024

Uh oh!

gchatelet Mar 19, 2024

Uh oh!

nickdesaulniers Mar 19, 2024

Uh oh!

nickdesaulniers Mar 19, 2024

Uh oh!

gchatelet Mar 19, 2024

Uh oh!

nickdesaulniers Mar 19, 2024

Uh oh!

nickdesaulniers Mar 19, 2024

Uh oh!

nickdesaulniers Mar 19, 2024

Uh oh!

nickdesaulniers Mar 19, 2024

Uh oh!

gchatelet Mar 20, 2024

Uh oh!

michaelrj-google commented Mar 19, 2024

Uh oh!

Uh oh!

[libc][cpp] reverse_iterator support #85702

[libc][cpp] reverse_iterator support #85702

Uh oh!

Conversation

nickdesaulniers commented Mar 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Mar 18, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelrj-google commented Mar 19, 2024

Uh oh!

Uh oh!

nickdesaulniers commented Mar 18, 2024 •

edited

Loading