[libc++][ranges] LWG4001: `iota_view` should provide `empty` #79687

H-G-Hristov · 2024-01-27T10:10:18Z

Implements: https://wg21.link/LWG4001

https://eel.is/c++draft/range.iota.view

llvmbot · 2024-01-27T11:22:19Z

@llvm/pr-subscribers-libcxx

Author: Hristo Hristov (H-G-Hristov)

Changes

Implements: https://wg21.link/LWG4001

https://eel.is/c++draft/range.iota.view

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

3 Files Affected:

(modified) libcxx/docs/Status/Cxx2cIssues.csv (+1-1)
(modified) libcxx/include/__ranges/iota_view.h (+2)
(added) libcxx/test/std/ranges/range.factories/range.iota.view/empty.pass.cpp (+136)

diff --git a/libcxx/docs/Status/Cxx2cIssues.csv b/libcxx/docs/Status/Cxx2cIssues.csv
index b69b0948325412..6401c7c6d8fc7d 100644
--- a/libcxx/docs/Status/Cxx2cIssues.csv
+++ b/libcxx/docs/Status/Cxx2cIssues.csv
@@ -38,7 +38,7 @@
 "`3974 <https://wg21.link/LWG3974>`__","``mdspan::operator[]`` should not copy ``OtherIndexTypes``","Kona November 2023","","",""
 "`3987 <https://wg21.link/LWG3987>`__","Including ``<flat_foo>`` doesn't provide ``std::begin``/``end``","Kona November 2023","","","|flat_containers|"
 "`3990 <https://wg21.link/LWG3990>`__","Program-defined specializations of ``std::tuple`` and ``std::variant`` can't be properly supported","Kona November 2023","","",""
-"`4001 <https://wg21.link/LWG4001>`__","``iota_view`` should provide ``empty``","Kona November 2023","","","|ranges|"
+"`4001 <https://wg21.link/LWG4001>`__","``iota_view`` should provide ``empty``","Kona November 2023","|Complete|","19.0","|ranges|"
 "","","","","",""
 "`3343 <https://wg21.link/LWG3343>`__","Ordering of calls to ``unlock()`` and ``notify_all()`` in Effects element of ``notify_all_at_thread_exit()`` should be reversed","Not Yet Adopted","|Complete|","16.0",""
 "","","","","",""
diff --git a/libcxx/include/__ranges/iota_view.h b/libcxx/include/__ranges/iota_view.h
index c8314dd848b447..6c5923c7ef013c 100644
--- a/libcxx/include/__ranges/iota_view.h
+++ b/libcxx/include/__ranges/iota_view.h
@@ -345,6 +345,8 @@ class iota_view : public view_interface<iota_view<_Start, _BoundSentinel>> {
     return __iterator{__bound_sentinel_};
   }
 
+  _LIBCPP_HIDE_FROM_ABI constexpr bool empty() const { return __value_ == __bound_sentinel_; }
+
   _LIBCPP_HIDE_FROM_ABI constexpr auto size() const
     requires(same_as<_Start, _BoundSentinel> && __advanceable<_Start>) ||
             (integral<_Start> && integral<_BoundSentinel>) || sized_sentinel_for<_BoundSentinel, _Start>
diff --git a/libcxx/test/std/ranges/range.factories/range.iota.view/empty.pass.cpp b/libcxx/test/std/ranges/range.factories/range.iota.view/empty.pass.cpp
new file mode 100644
index 00000000000000..e7adffaf63b4c9
--- /dev/null
+++ b/libcxx/test/std/ranges/range.factories/range.iota.view/empty.pass.cpp
@@ -0,0 +1,136 @@
+//===----------------------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+
+// constexpr bool empty() const;
+
+#include <cassert>
+#include <concepts>
+#include <ranges>
+#include <utility>
+#include <vector>
+
+#include "types.h"
+
+template <typename R>
+concept HasFreeEmpty = requires(R r) { std::ranges::empty(r); };
+
+template <typename R>
+concept HasMemberEmpty = requires(R r) {
+  { r.empty() } -> std::same_as<bool>;
+};
+
+constexpr void test_empty_iota() {
+  std::vector<int> ev;
+
+  // Both parameters are non-const
+  {
+    auto iv = std::views::iota(std::ranges::begin(ev), std::ranges::end(ev));
+
+    static_assert(HasFreeEmpty<decltype(iv)>);
+    static_assert(HasMemberEmpty<decltype(iv)>);
+
+    assert(iv.empty());
+  }
+  // Left paramter is const
+  {
+    auto iv = std::views::iota(std::ranges::begin(std::as_const(ev)), std::ranges::end(ev));
+
+    static_assert(HasFreeEmpty<decltype(iv)>);
+    static_assert(HasMemberEmpty<decltype(iv)>);
+
+    assert(iv.empty());
+  }
+  // Right paramter is const
+  {
+    auto iv = std::views::iota(std::ranges::begin(ev), std::ranges::end(std::as_const(ev)));
+
+    static_assert(HasFreeEmpty<decltype(iv)>);
+    static_assert(HasMemberEmpty<decltype(iv)>);
+
+    assert(iv.empty());
+  }
+  // Both parameters are const
+  {
+    auto iv = std::views::iota(std::ranges::begin(std::as_const(ev)), std::ranges::end(std::as_const(ev)));
+
+    static_assert(HasFreeEmpty<decltype(iv)>);
+    static_assert(HasMemberEmpty<decltype(iv)>);
+
+    assert(iv.empty());
+  }
+
+  std::vector<char> v{'b', 'a', 'b', 'a', 'z', 'm', 't'};
+  auto fv = v | std::views::filter([](auto val) { return val == '0'; });
+
+  {
+    auto iv = std::views::iota(std::ranges::begin(fv), std::ranges::end(fv));
+
+    static_assert(HasFreeEmpty<decltype(iv)>);
+    static_assert(HasMemberEmpty<decltype(iv)>);
+
+    assert(iv.empty());
+  }
+}
+
+constexpr void test_nonempty_iota() {
+  // Default ctr
+  {
+    std::ranges::iota_view<Int42<DefaultTo42>> iv;
+
+    static_assert(HasFreeEmpty<decltype(iv)>);
+    static_assert(HasMemberEmpty<decltype(iv)>);
+
+    assert(!iv.empty());
+  }
+  // Value pass
+  {
+    std::ranges::iota_view<SomeInt> iv(SomeInt(94));
+
+    static_assert(HasFreeEmpty<decltype(iv)>);
+    static_assert(HasMemberEmpty<decltype(iv)>);
+
+    assert(!iv.empty());
+  }
+
+  {
+    std::vector<char> v;
+    auto it = std::back_inserter(v);
+    auto iv = std::views::iota(it);
+
+    static_assert(HasFreeEmpty<decltype(iv)>);
+    static_assert(HasMemberEmpty<decltype(iv)>);
+
+    assert(!iv.empty());
+  }
+  {
+    std::vector<char> v{'b', 'a', 'b', 'a', 'z', 'm', 't'};
+    auto it = std::back_inserter(v);
+    auto iv = std::views::iota(it);
+
+    static_assert(HasFreeEmpty<decltype(iv)>);
+    static_assert(HasMemberEmpty<decltype(iv)>);
+
+    assert(!iv.empty());
+  }
+}
+
+constexpr bool test() {
+  test_empty_iota();
+  test_nonempty_iota();
+
+  return true;
+}
+
+int main(int, char**) {
+  test();
+  static_assert(test());
+
+  return 0;
+}

hawkinsw

Hope these little suggestions help! Thanks for doing the work!

libcxx/test/std/ranges/range.factories/range.iota.view/empty.pass.cpp

var-const

Sorry about the slow review! Almost LGTM with just a couple of nits.

var-const · 2024-03-31T20:38:49Z

libcxx/test/std/ranges/range.factories/range.iota.view/empty.pass.cpp

+  {
+    auto iv = std::views::iota(std::ranges::begin(ev), std::ranges::end(ev));
+
+    static_assert(HasFreeEmpty<decltype(iv)>);


Nit: normally, the ranges tests follow a pattern where constraints are tested in a separate block before testing the actual function -- can you please do the same? It's mostly for consistency, but IMO it's also easier to make sure we're testing all the constraints when the tests are all next to each other and not spread out in the file.

libcxx/test/std/ranges/range.factories/range.iota.view/empty.pass.cpp

var-const · 2024-03-31T21:03:26Z

libcxx/test/std/ranges/range.factories/range.iota.view/empty.pass.cpp

+};
+
+constexpr void test_empty_iota() {
+  std::vector<int> ev;


Question: what do ev and iv stand for?

Normally I prefer long descriptive names but the libc++ tests use short names, so I try to be consistent:
ev -> empty vector vs v -> non-empty vector.
iv -> iota_view

I can change these.

var-const · 2024-03-31T21:03:59Z

libcxx/test/std/ranges/range.factories/range.iota.view/empty.pass.cpp

+  }
+  // Right parameter is const
+  {
+    auto iv = std::views::iota(std::ranges::begin(ev), std::ranges::end(std::as_const(ev)));


Question: does using as_const really matter to test iv.empty()?

I thought it was good to be detailed. Should I remove those?

libcxx/test/std/ranges/range.factories/range.iota.view/empty.pass.cpp

Implements: https://wg21.link/LWG4001 - https://eel.is/c++draft/range.iota.view

…ass.cpp Co-authored-by: Will Hawkins <whh8b@obs.cr>

…empty

ldionne

Minor comments about the tests but this overall LGTM.

ldionne · 2024-07-15T15:29:30Z

libcxx/test/std/ranges/range.factories/range.iota.view/empty.pass.cpp

+#include "types.h"
+
+template <typename R>
+concept HasEmpty = requires(R r) {


Suggested change

concept HasEmpty = requires(R r) {

concept HasEmpty = requires(R const r) {

Otherwise you don't test anywhere that the method is marked as const.

ldionne · 2024-07-15T15:31:54Z

libcxx/test/std/ranges/range.factories/range.iota.view/empty.pass.cpp

+  // Left parameter is const
+  {
+    auto iv = std::views::iota(std::ranges::begin(std::as_const(ev)), std::ranges::end(ev));
+
+    assert(iv.empty());
+  }
+  // Right parameter is const
+  {
+    auto iv = std::views::iota(std::ranges::begin(ev), std::ranges::end(std::as_const(ev)));
+
+    assert(iv.empty());
+  }
+  // Both parameters are const
+  {
+    auto iv = std::views::iota(std::ranges::begin(std::as_const(ev)), std::ranges::end(std::as_const(ev)));
+
+    assert(iv.empty());
+  }


I don't really understand the purpose of these tests. We're interested in checking that empty() can be called on a const iota_view, not that we can call it on a mutable iota_view constructed from const iterators?

…empty

H-G-Hristov · 2024-07-15T17:37:27Z

Minor comments about the tests but this overall LGTM.

Thanks! I think I addressed the comments.

ldionne

LGTM. Can be merged once CI is green. Thanks!

Outdated diff.

H-G-Hristov · 2024-07-17T15:07:59Z

Thank you! The CI is green finally!

) Implements: https://wg21.link/LWG4001 - https://eel.is/c++draft/range.iota.view --------- Co-authored-by: Zingam <zingam@outlook.com> Co-authored-by: Will Hawkins <whh8b@obs.cr>

Summary: Implements: https://wg21.link/LWG4001 - https://eel.is/c++draft/range.iota.view --------- Co-authored-by: Zingam <zingam@outlook.com> Co-authored-by: Will Hawkins <whh8b@obs.cr> Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60251523

H-G-Hristov marked this pull request as ready for review January 27, 2024 11:21

H-G-Hristov requested a review from a team as a code owner January 27, 2024 11:21

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Jan 27, 2024

hawkinsw reviewed Jan 27, 2024

View reviewed changes

libcxx/test/std/ranges/range.factories/range.iota.view/empty.pass.cpp Outdated Show resolved Hide resolved

libcxx/test/std/ranges/range.factories/range.iota.view/empty.pass.cpp Outdated Show resolved Hide resolved

ldionne added the ranges Issues related to `<ranges>` label Mar 7, 2024

var-const self-assigned this Mar 8, 2024

Zingam mentioned this pull request Mar 10, 2024

[libc++]Implement LWG4001(iota_view should provide empty) #84669

Closed

var-const previously requested changes Mar 31, 2024

View reviewed changes

Zingam and others added 4 commits April 2, 2024 16:38

[libc++][ranges] LWG4001: iota_view should provide empty

fe76be2

Implements: https://wg21.link/LWG4001 - https://eel.is/c++draft/range.iota.view

Update libcxx/test/std/ranges/range.factories/range.iota.view/empty.p…

bdb67fc

…ass.cpp Co-authored-by: Will Hawkins <whh8b@obs.cr>

Update libcxx/test/std/ranges/range.factories/range.iota.view/empty.p…

b76e34c

…ass.cpp Co-authored-by: Will Hawkins <whh8b@obs.cr>

Addressed comments

02c4e71

H-G-Hristov force-pushed the hgh/libcxx/LWG4001-iota_view_should_provide_empty branch from 76c72a5 to 02c4e71 Compare April 2, 2024 13:39

H-G-Hristov requested a review from var-const April 15, 2024 06:32

ldionne added this to the LLVM 19.X Release milestone Jul 9, 2024

Merge branch 'main' into hgh/libcxx/LWG4001-iota_view_should_provide_…

d769525

…empty

ldionne requested changes Jul 15, 2024

View reviewed changes

H-G-Hristov added 2 commits July 15, 2024 20:35

Addressed review comments

8b1131f

Merge branch 'main' into hgh/libcxx/LWG4001-iota_view_should_provide_…

40fa938

…empty

ldionne approved these changes Jul 15, 2024

View reviewed changes

Zingam merged commit 60b6f43 into llvm:main Jul 17, 2024
58 checks passed

vitalybuka mentioned this pull request Jul 18, 2024

Some patches are missing from bots "Changes" llvm/llvm-zorg#233

Closed

H-G-Hristov deleted the hgh/libcxx/LWG4001-iota_view_should_provide_empty branch July 19, 2024 15:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[libc++][ranges] LWG4001: `iota_view` should provide `empty` #79687

[libc++][ranges] LWG4001: `iota_view` should provide `empty` #79687

H-G-Hristov commented Jan 27, 2024

llvmbot commented Jan 27, 2024

hawkinsw left a comment

var-const left a comment

var-const Mar 31, 2024

var-const Mar 31, 2024

H-G-Hristov Apr 2, 2024

var-const Mar 31, 2024

H-G-Hristov Apr 15, 2024

H-G-Hristov Jul 15, 2024

ldionne left a comment

ldionne Jul 15, 2024

H-G-Hristov Jul 15, 2024

ldionne Jul 15, 2024

H-G-Hristov Jul 15, 2024

H-G-Hristov commented Jul 15, 2024

ldionne left a comment

H-G-Hristov commented Jul 17, 2024

	concept HasEmpty = requires(R r) {
	concept HasEmpty = requires(R const r) {

[libc++][ranges] LWG4001: iota_view should provide empty #79687

[libc++][ranges] LWG4001: iota_view should provide empty #79687

Conversation

H-G-Hristov commented Jan 27, 2024

llvmbot commented Jan 27, 2024

hawkinsw left a comment

Choose a reason for hiding this comment

var-const left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldionne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

H-G-Hristov commented Jul 15, 2024

ldionne left a comment

Choose a reason for hiding this comment

H-G-Hristov commented Jul 17, 2024

[libc++][ranges] LWG4001: `iota_view` should provide `empty` #79687

[libc++][ranges] LWG4001: `iota_view` should provide `empty` #79687