Make `contiguous_iterator` constexpr iterator by frederick-vs-ja · Pull Request #64 · bemanproject/optional

frederick-vs-ja · 2024-09-25T02:48:55Z

Per [optional.iterators]/1 and the proposed [optionalref.iterators]/1 in P2988R7.

Rough test: https://godbolt.org/z/7eEhh954W

Fixes #63.

Per [optional.iterators]/1 and the proposed [optionalref.iterators]/1 in P2988R7. Rough test: https://godbolt.org/z/7eEhh954W

frederick-vs-ja · 2024-09-25T02:51:27Z

include/beman/optional26/detail/iterator.hpp


    // Pointer to iterator constructor.
-    contiguous_iterator(pointer it) noexcept : m_current(it) {}
+    constexpr contiguous_iterator(pointer it) noexcept : m_current(it) {}


This ctor currently allows implicit conversion, which in turn allows more permissive operations between contiguous_iterator and raw pointer.
Should we disallow such implicit conversion, or even furtherly disallow explicit construction? (Perhaps should do in another PR.)

I've got a branch with updated wording and a number of fixes to conversion and dangling issues. It's exactly the current proposal. Getting the constraints and mandates to all play nice was a little tricky, but we did notice that assignment boiled down to one assignment from tuple, and that the move and copy assignments were identical, so we only need one.

It is using the newish reference_converts_from_temporary the way that tuple does now.

PR will be up later today.

The branch currently using aliases the traits that it uses into the Optional26 namespace in order to get the wording synopsis, constraints, mandates, effects all correct and in a form that I can just paste into the TeX doc for standardization, but probably shouldn't be done for the production version.

It's also based on the pre renaming version.

The really good news is that an LWG expert helped get it right, so this should be much smoother in the LEWG approval of wording and actual LWG review.

include/beman/optional26/detail/iterator.hpp

camio

Thanks for the contribution @frederick-vs-ja! This change looks good to me. Can we get a unit test that verifies the changes work as expected?

@steve-downey are you suggesting we hold off on merging this PR in anticipation of your bigger fix?

neatudarius · 2024-09-30T06:21:50Z

@frederick-vs-ja , we would like to proceed with your PR. If new paper revision will change iterator interface at a later point, we'll deal with that in other PR.

can you applied the required feedback on this PR?

CC: @camio @steve-downey , let's allow this improvement in main. Thanks @frederick-vs-ja for contribution into optional26!

src/beman/optional26/tests/optional_range_support.t.cpp

JeffGarland · 2024-10-01T15:21:20Z

@frederick-vs-ja , we would like to proceed with your PR. If new paper revision will change iterator interface at a later point, we'll deal with that in other PR.

can you applied the required feedback on this PR?

CC: @camio @steve-downey , let's allow this improvement in main. Thanks @frederick-vs-ja for contribution into optional26!

Concur here -- big steps forward on constexpr, which can be surprisingly tricky.

Which make the workaround for https://gcc.gnu.org/PR89074 and P1004R2 `constexpr` `std::vector` unnecessary which `constexpr` test coverage enabled.

camio · 2024-10-02T16:55:53Z

src/beman/optional26/tests/detail/iterator.t.cpp

 #include <vector>

-namespace beman::optional26::test {
+#define CONSTEXPR_EXPECT_EQ(val1, val2)   \


I don't understand why this CONSTEXPR_ set of functions was introduced. Should we use static_assert be used to check the constexpr functionality?

I'm already using static_assert((lambda(), true)); now. These set of wrapping macros were introduced because gtest's corresponding macros are not constexpr-friendly.

First of all, this seems to be duplicated code from src/beman/optional26/tests/optional_range_support.t.cpp. So if we'll keep it, @frederick-vs-ja please move it in est_utilities/test_types.*.

2nd observation: I think we want to have all constexpr tests in a separate file, similar to https://github.com/beman-project/optional26/blob/main/src/beman/optional26/tests/optional_constexpr.t.cpp (optional.t.cpp vs optional_constexpr.t.cpp). @camio , @steve-downey ?

3rd: I think static_assert should work. IF not, this is how we do constexpr testing right now - https://github.com/beman-project/optional26/blob/main/src/beman/optional26/tests/optional_constexpr.t.cpp#L251-L263. Hope it hepls.

I'm already using static_assert((lambda(), true)); now. These set of wrapping macros were introduced because gtest's corresponding macros are not constexpr-friendly.

Oh, I see. You're doing this so you can use something like EXPECT_EQ from within a lambda evaluated at compile time. Cool.

neatudarius · 2024-10-02T17:04:17Z

src/beman/optional26/tests/test_types.hpp

 struct move_detector {
    move_detector() = default;
-    move_detector(move_detector&& rhs) { rhs.been_moved = true; }
+    constexpr move_detector(move_detector&& rhs) { rhs.been_moved = true; }


Should we add constexpr to other/all constructors in this class/file?

The main reason for the duplication was that I duplicated all the tests to start with, and then worked to consteval them. It made it a little easier to track progress, but I wouldn't make it a mandatory pattern, necessarily.
If it were consteval first, it might depend on implementation of the non-const version needs separate testing?

neatudarius · 2024-10-02T17:09:44Z

src/beman/optional26/tests/detail/iterator.t.cpp

 #include <vector>

-namespace beman::optional26::test {
+#define CONSTEXPR_EXPECT_EQ(val1, val2)   \


First of all, this seems to be duplicated code from src/beman/optional26/tests/optional_range_support.t.cpp. So if we'll keep it, @frederick-vs-ja please move it in est_utilities/test_types.*.

2nd observation: I think we want to have all constexpr tests in a separate file, similar to https://github.com/beman-project/optional26/blob/main/src/beman/optional26/tests/optional_constexpr.t.cpp (optional.t.cpp vs optional_constexpr.t.cpp). @camio , @steve-downey ?

3rd: I think static_assert should work. IF not, this is how we do constexpr testing right now - https://github.com/beman-project/optional26/blob/main/src/beman/optional26/tests/optional_constexpr.t.cpp#L251-L263. Hope it hepls.

neatudarius

LGTM 👍

Make contiguous_iterator constexpr iterator

9c2f33e

Per [optional.iterators]/1 and the proposed [optionalref.iterators]/1 in P2988R7. Rough test: https://godbolt.org/z/7eEhh954W

frederick-vs-ja requested review from camio, neatudarius and steve-downey as code owners September 25, 2024 02:48

frederick-vs-ja commented Sep 25, 2024

View reviewed changes

neatudarius reviewed Sep 25, 2024

View reviewed changes

include/beman/optional26/detail/iterator.hpp Show resolved Hide resolved

include/beman/optional26/detail/iterator.hpp Show resolved Hide resolved

camio reviewed Sep 26, 2024

View reviewed changes

neatudarius self-assigned this Sep 30, 2024

Expand constexpr tests

1097ea2

neatudarius reviewed Oct 1, 2024

View reviewed changes

src/beman/optional26/tests/optional_range_support.t.cpp Outdated Show resolved Hide resolved

JeffGarland reviewed Oct 1, 2024

View reviewed changes

src/beman/optional26/tests/optional_range_support.t.cpp Outdated Show resolved Hide resolved

JeffGarland self-requested a review October 1, 2024 15:19

JeffGarland approved these changes Oct 1, 2024

View reviewed changes

Drop GCC11 from ci

955f74d

Which make the workaround for https://gcc.gnu.org/PR89074 and P1004R2 `constexpr` `std::vector` unnecessary which `constexpr` test coverage enabled.

frederick-vs-ja force-pushed the constexpr-iterator branch from a28cac5 to 955f74d Compare October 2, 2024 06:42

frederick-vs-ja requested a review from camio October 2, 2024 16:12

camio reviewed Oct 2, 2024

View reviewed changes

neatudarius reviewed Oct 2, 2024

View reviewed changes

neatudarius approved these changes Oct 15, 2024

View reviewed changes

steve-downey merged commit 105f209 into bemanproject:main Oct 15, 2024

Conversation

frederick-vs-ja commented Sep 25, 2024

Uh oh!

frederick-vs-ja Sep 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Uh oh!

camio left a comment

Choose a reason for hiding this comment

Uh oh!

neatudarius commented Sep 30, 2024

Uh oh!

Uh oh!

Uh oh!

JeffGarland commented Oct 1, 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!

neatudarius left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

frederick-vs-ja Sep 25, 2024 •

edited

Loading