Skip to content

Conversation

@frederick-vs-ja
Copy link
Contributor

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

// Pointer to iterator constructor.
contiguous_iterator(pointer it) noexcept : m_current(it) {}
constexpr contiguous_iterator(pointer it) noexcept : m_current(it) {}
Copy link
Contributor Author

@frederick-vs-ja frederick-vs-ja Sep 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@camio camio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

@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!

@neatudarius neatudarius self-assigned this Sep 30, 2024
@JeffGarland JeffGarland self-requested a review October 1, 2024 15:19
@JeffGarland
Copy link
Member

@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.
#include <vector>

namespace beman::optional26::test {
#define CONSTEXPR_EXPECT_EQ(val1, val2) \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@neatudarius neatudarius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@steve-downey steve-downey merged commit 105f209 into bemanproject:main Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

contiguous_iterator should be constexpr

5 participants