Make contiguous_iterator constexpr iterator
#64
Conversation
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
|
|
||
| // Pointer to iterator constructor. | ||
| contiguous_iterator(pointer it) noexcept : m_current(it) {} | ||
| constexpr contiguous_iterator(pointer it) noexcept : m_current(it) {} |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
camio
left a comment
camio
left a comment
There was a problem hiding this comment.
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?
|
@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.
frederick-vs-ja
force-pushed
the
constexpr-iterator
branch
from
a28cac5 to
955f74d
Compare
| #include <vector> | ||
|
|
||
| namespace beman::optional26::test { | ||
| #define CONSTEXPR_EXPECT_EQ(val1, val2) \ |
There was a problem hiding this comment.
I don't understand why this CONSTEXPR_ set of functions was introduced. Should we use static_assert be used to check the constexpr functionality?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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; } |
There was a problem hiding this comment.
Should we add constexpr to other/all constructors in this class/file?
There was a problem hiding this comment.
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?
| #include <vector> | ||
|
|
||
| namespace beman::optional26::test { | ||
| #define CONSTEXPR_EXPECT_EQ(val1, val2) \ |
There was a problem hiding this comment.
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.
5 participants
Per [optional.iterators]/1 and the proposed [optionalref.iterators]/1 in P2988R7.
Rough test: https://godbolt.org/z/7eEhh954W
Fixes #63.