R9 of paper which fixes the reference stealing bug #103
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fix the construction and assignment of optional<U&>&& to an optional<T> where the referred to value could be stolen.
Improve the parallelism in t he declarations in the implementation to clarify that the difference in the overloads is due to the 'Other' parameter for removing it from consideration for overload since the U parameter is *already* a reference. Checking for 'const U&' is no longer the right thing to check.
neatudarius
approved these changes
Jan 20, 2025
Member
neatudarius
left a comment
neatudarius
left a comment
There was a problem hiding this comment.
This looks great! Thanks for improving std/beman optional! ;)
| The proposed changes are relative to the current working draft \cite{N4910}. | ||
|
|
||
| \chapter{Acknowledgements} | ||
| Many thanks to all of the reviewers and authors of beman/optional26, \cite{The_Beman_Project_beman_optional26}, in particular A. Jiang, Darius Neațu, David Sankel, Eddie Nolan, Jan Kokemüller, Jeff Garland, and River. Tomasz Kamiński provided extensive support for the library wording of optional<T\&>. |
Member
There was a problem hiding this comment.
Suggested change
| Many thanks to all of the reviewers and authors of beman/optional26, \cite{The_Beman_Project_beman_optional26}, in particular A. Jiang, Darius Neațu, David Sankel, Eddie Nolan, Jan Kokemüller, Jeff Garland, and River. Tomasz Kamiński provided extensive support for the library wording of optional<T\&>. | |
| Many thanks to all of the reviewers and authors of beman/optional, \cite{The_Beman_Project_beman_optional26}, in particular A. Jiang, Darius Neațu, David Sankel, Eddie Nolan, Jan Kokemüller, Jeff Garland, and River. Tomasz Kamiński provided extensive support for the library wording of optional<T\&>. |
It's up to you if you want to postpone the renaming from #102 until Austria. If already planning to merge #102 first, reminder to update this branch.
And also, thanks for acknowledge here! :D
| author = {The Beman Project}, | ||
| license = {Apache-2.0}, | ||
| title = {{beman.optional26}}, | ||
| howpublished = {\url{https://github.com/bemanproject/optional26}}, |
Member
There was a problem hiding this comment.
Same reminder here about renaming. Feel free to ignore it. Just trying to help for Austria.
| } | ||
|
|
||
| /// Converting move constructor. | ||
| /// Converting copy constructor for U& |
Member
There was a problem hiding this comment.
Can we please add some tests to cover this bugfix? Thanks for fixing it!
Member
Author
|
The paper had to go out last Monday, so this is catch up. I'll update the
name of the project and repo in follow up, and for the eventual final
wording paper. This version gets a nod from LEWG Tuesday (fingers crossed).
I'll add some further test cases, too, although I believe Jan's patch had
some initial ones.
…On Mon, Jan 20, 2025 at 11:39 AM Darius Neațu ***@***.***> wrote:
***@***.**** approved this pull request.
This looks great! Thanks for improving std/beman optional! ;)
------------------------------
In papers/P2988/optional_ref_wording.tex
<#103 (comment)>
:
> @@ -406,6 +413,10 @@ \chapter{Impact on the standard}
The proposed changes are relative to the current working draft \cite{N4910}.
+\chapter{Acknowledgements}
+Many thanks to all of the reviewers and authors of beman/optional26, \cite{The_Beman_Project_beman_optional26}, in particular A. Jiang, Darius Neațu, David Sankel, Eddie Nolan, Jan Kokemüller, Jeff Garland, and River. Tomasz Kamiński provided extensive support for the library wording of optional<T\&>.
⬇️ Suggested change
-Many thanks to all of the reviewers and authors of beman/optional26, \cite{The_Beman_Project_beman_optional26}, in particular A. Jiang, Darius Neațu, David Sankel, Eddie Nolan, Jan Kokemüller, Jeff Garland, and River. Tomasz Kamiński provided extensive support for the library wording of optional<T\&>.
+Many thanks to all of the reviewers and authors of beman/optional, \cite{The_Beman_Project_beman_optional26}, in particular A. Jiang, Darius Neațu, David Sankel, Eddie Nolan, Jan Kokemüller, Jeff Garland, and River. Tomasz Kamiński provided extensive support for the library wording of optional<T\&>.
It's up to you if you want to postpone the renaming from #102
<#102> until Austria. If
already planning to merge #102
<#102> first, reminder to
update this branch.
And also, thanks for acknowledge here! :D
------------------------------
In papers/P2988/mybiblio.bib
<#103 (comment)>
:
> @@ -58,3 +58,12 @@ @misc{rawgithu58:online
year = {},
note = {(Accessed on 08/14/2024)}
}
+
***@***.***{The_Beman_Project_beman_optional26,
+author = {The Beman Project},
+license = {Apache-2.0},
+title = {{beman.optional26}},
+howpublished = {\url{https://github.com/bemanproject/optional26}},
Same reminder here about renaming. Feel free to ignore it. Just trying to
help for Austria.
------------------------------
In include/beman/optional26/optional.hpp
<#103 (comment)>
:
> @@ -470,24 +474,25 @@ inline constexpr optional<T>::optional(const optional<U>& rhs)
}
}
-/// Converting move constructor.
+/// Converting copy constructor for U&
Can we please add some tests to cover this bugfix? Thanks for fixing it!
—
Reply to this email directly, view it on GitHub
<#103 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAVNZ5RKJY6APHNBKPCZSFD2LURFNAVCNFSM6AAAAABVOAONVOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDKNRSHAYDSNJUHE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Paper includes fix for an rvalue-ref optional<T&> being assigned to an optional and stealing via move the referred to object.