Skip to content

Fix some bugs in optional<T&>; add tests for them#58

Merged
steve-downey merged 9 commits intobemanproject:mainfrom
Quuxplusone:fix-some-bugs
Nov 4, 2024
Merged

Fix some bugs in optional<T&>; add tests for them#58
steve-downey merged 9 commits intobemanproject:mainfrom
Quuxplusone:fix-some-bugs

Conversation

@Quuxplusone
Copy link
Contributor

@Quuxplusone Quuxplusone commented Sep 16, 2024

There's a lot of commits in this PR — feel free to cherry-pick (but I think they all need to be taken/addressed, one way or another). The really important ones are:

commit 4448d97dbd98e017203931e4a9f9a5a8ddb831e8
Author: Arthur O'Dwyer <arthur.j.odwyer@gmail.com>
Date:   Mon Sep 16 13:06:42 2024 -0400

    [test] Add a green test for overload resolution

commit 986b975ad718613351aa1fe4ce20d75e6f956c41
Author: Arthur O'Dwyer <arthur.j.odwyer@gmail.com>
Date:   Mon Sep 16 12:20:19 2024 -0400

    Support optional<T&>::emplace, and add tests
    
    The return type of `emplace` had been wrong; `emplace` returns a
    reference to the emplaced value (in this case, the T& reference
    that was bound), not `*this`.
    
    The new tests are red before the patch and green afterward.

commit a6333d30c8a09c38dae8577f67181a74dbb34e4b
Author: Arthur O'Dwyer <arthur.j.odwyer@gmail.com>
Date:   Mon Sep 16 12:11:47 2024 -0400

    Support binding optional<const T&> to a non-const T, and add tests
    
    The new tests are red before the patch and green afterward.

commit cd93469568a509ac3d2d18904df95e981b196136
Author: Arthur O'Dwyer <arthur.j.odwyer@gmail.com>
Date:   Mon Sep 16 11:29:36 2024 -0400

    Support optional<optional<int>&>
    
    Add two new tests: one for `optional<optional<int>&>`, and one
    testing that we can construct and assign to `optional<T&>` from
    `reference_wrapper<T>` (which should work: since `reference_wrapper<T>`
    is implicitly convertible to `T&`, it also should be implicitly
    convertible to `optional<T&>`).
    These tests are both red before this patch and green afterward.

These also correspond to bugs and/or typos in P2988's wording; for example it has emplace returning optional& instead of T&.

@Quuxplusone
[CI] Allow users to manually trigger the CI and precommit workflows
83674c8 
https://docs.github.com/en/actions/managing-workflow-runs-and-deployments/managing-workflow-runs/manually-running-a-workflow
https://stackoverflow.com/questions/76363988/github-actions-workflow-dispatch-event-triggers-automatically-on-pull-request
@Quuxplusone
[test] Fix warnings from Clang trunk
50d8616 
`static_cast` to lvalue suppresses Clang's self-assignment warning.
Add (missing?) EXPECTs to silence Clang's unused-variable warning.
Add an accessor to suppress Clang's unused-private-field warning.
Drive-by rename "empty_base" to "disengaged_base", since the phrase
"empty base" means something different in C++.
@Quuxplusone
Eliminate redundant constexpr/noexcept keywords. NFC.
926663c
@Quuxplusone
[test] Add a green test for optional<optional>. NFC.
eba41fd
@Quuxplusone
Support optional<optional<int>&>
cd93469 
Add two new tests: one for `optional<optional<int>&>`, and one
testing that we can construct and assign to `optional<T&>` from
`reference_wrapper<T>` (which should work: since `reference_wrapper<T>`
is implicitly convertible to `T&`, it also should be implicitly
convertible to `optional<T&>`).
These tests are both red before this patch and green afterward.
@Quuxplusone
[test] Add a green test for optional<optional>::emplace
fbf4b1c
@Quuxplusone
Support binding optional<const T&> to a non-const T, and add tests
a6333d3 
The new tests are red before the patch and green afterward.
@Quuxplusone
Support optional<T&>::emplace, and add tests
986b975 
The return type of `emplace` had been wrong; `emplace` returns a
reference to the emplaced value (in this case, the T& reference
that was bound), not `*this`.

The new tests are red before the patch and green afterward.
@Quuxplusone
[test] Add a green test for overload resolution
4448d97
@steve-downey
Copy link
Member

steve-downey commented Sep 16, 2024 via email

@steve-downey
Copy link
Member

steve-downey commented Sep 16, 2024

I've merged main and the additional tests. The changes to optional.hpp need to be done piece by piece as the diffs are large enough to confuse git.

@camio
Copy link
Member

camio commented Sep 17, 2024

Thanks for the contributions @Quuxplusone, and welcome!

@camio
Copy link
Member

camio commented Oct 2, 2024

@steve-downey/@neatudarius, what is the status of this PR? Were these changes applied in a different merge?

@steve-downey
Copy link
Member

steve-downey commented Oct 2, 2024

They are incorporated into steve-downey#19
Which will be a PR into https://github.com/beman-project/optional26/ soon.

@Quuxplusone
Copy link
Contributor Author

Quuxplusone commented Oct 3, 2024

@camio Also, FWIW, I believe we've established that my new test OverloadResolutionChecksDangling is supposed to be red — that is, I think it should go green, but @steve-downey and Tomasz say it was designed not-to-work on purpose (for consistency with how it doesn't-work for tuple<T&> either). That, in turn, means that my new helpers safely_convertible and safely_constructible are kind of pointless. So in short I think Steve and I are on the same page that this PR isn't trivially mergeable, and I'm still unclear as to which parts of it are uncontroversial.

E.g. I think it's uncontroversial that the return type of emplace should be T&, but I'm not even 100% sure we're on the same page about that, necessarily.

@steve-downey steve-downey merged commit a3998c8 into bemanproject:main Nov 4, 2024
steve-downey added a commit that referenced this pull request Feb 8, 2026
@steve-downey
Merge pull request #58 from steve-downey/dependabot/github_actions/gi…
ac6ba95 
…thub-owned-actions-4d5d113fee

Bump github/codeql-action from 4.31.11 to 4.32.2 in the github-owned-actions group
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@camio camio Awaiting requested review from camio camio is a code owner

@neatudarius neatudarius Awaiting requested review from neatudarius neatudarius is a code owner

@steve-downey steve-downey Awaiting requested review from steve-downey steve-downey is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Quuxplusone @steve-downey @camio