Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AK/Optional: Make some member functions constexpr #24978

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

nrdmn
Copy link

@nrdmn nrdmn commented Aug 31, 2024

This PR changes some of Optional's member functions to be constexpr - for now only the non-reference variant. Notably, this requires using construct_at instead of placement new to create the contained object and storing it in a union instead of a byte buffer.

@github-actions github-actions bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label Aug 31, 2024
@nrdmn nrdmn changed the title AK/Optional: Make some member functions constexpr WIP: AK/Optional: Make some member functions constexpr Aug 31, 2024
Copy link
Contributor

@Hendiadyoin1 Hendiadyoin1 left a comment

Choose a reason for hiding this comment

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

Generally don't like the hoops the standard makes us jump through, but that's not a blocking issue

Great change anyway, did you notice any changes in codesize or the like?

AK/Optional.h Show resolved Hide resolved
AK/Optional.h Outdated Show resolved Hide resolved
@nrdmn
Copy link
Author

nrdmn commented Aug 31, 2024

This PR currently breaks at Userland/Applications/PixelPaint/Tools/MoveTool.cpp:63 because there's an assignment to an Optional<const ....>. But so would std::optional: https://godbolt.org/z/z8c68e94Y

@Hendiadyoin1
Copy link
Contributor

Thaat looks like a logic bug, feel free to fix it

@nrdmn
Copy link
Author

nrdmn commented Aug 31, 2024

squashed the 'fixup' commits

@nrdmn nrdmn changed the title WIP: AK/Optional: Make some member functions constexpr AK/Optional: Make some member functions constexpr Aug 31, 2024
Copy link
Member

@BertalanD BertalanD left a comment

Choose a reason for hiding this comment

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

I generally like the idea, thanks! Please see the review comment.

Also, please add a test to ensure this is actually usable in a constexpr/consteval context.

AK/Optional.h Show resolved Hide resolved
@nrdmn nrdmn force-pushed the optional branch 2 times, most recently from 802df7a to d5961c5 Compare September 1, 2024 17:33
@nrdmn nrdmn requested a review from BertalanD September 1, 2024 19:59
construct_at has to be defined in the std namespace because outside of
it, compilers wouldn't allow it to be constexpr because of its use of
placement new.
This will allow Optional's Ctors to be constexpr.

Switching Optional's m_storage to be a union member is necessary if we
want to avoid pointer conversions, which we won't be allowed to do when
those functions are constexpr.
@Hendiadyoin1
Copy link
Contributor

With films work on Optional<T&>, this now has conficts

@Hendiadyoin1 Hendiadyoin1 added the ⚠️ pr-has-conflicts PR has merge conflicts and needs to be rebased label Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚠️ pr-has-conflicts PR has merge conflicts and needs to be rebased 👀 pr-needs-review PR needs review from a maintainer or community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants