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

build: Remove usage of deprecated std::aligned_storage/union #9297

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

victorvianna
Copy link
Contributor

These are deprecated in c++23.

Note: in the one instance where the default Align value was used for std::aligned_storage<Len, Alignment> (i.e.
std::aligned_storage), this patch now adopts
alignof(std::max_align_t). According to [1] that should be a behavior change, but in practice it doesn't seem to be when using clang nor gcc [2]. In any case, any change in the alignment would be an increase, which is fine.

[1] https://en.cppreference.com/w/cpp/types/aligned_storage
[2] https://godbolt.org/z/K5xGcxhjh

@victorvianna victorvianna requested a review from a team as a code owner January 23, 2025 15:52
@ci-tester-lunarg
Copy link
Collaborator

Author victorvianna not on autobuild list. Waiting for curator authorization before starting CI build.

@ci-tester-lunarg
Copy link
Collaborator

Author victorvianna not on autobuild list. Waiting for curator authorization before starting CI build.

@@ -174,7 +133,7 @@ using is_invocable_r = is_invocable_r_impl<void, R, F, Args...>;
} // namespace inplace_function_detail

template <class Signature, size_t Capacity = inplace_function_detail::InplaceFunctionDefaultCapacity,
size_t Alignment = alignof(inplace_function_detail::aligned_storage_t<Capacity>)>
size_t Alignment = alignof(std::max_align_t)>
Copy link
Contributor

Choose a reason for hiding this comment

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

Alignment will now always be the strictest, it can be improved it seems by replacing usage of aligned_storage with something similar to what is described here: https://stackoverflow.com/questions/71828288/why-is-stdaligned-storage-to-be-deprecated-in-c23-and-what-to-use-instead

Copy link
Contributor Author

@victorvianna victorvianna Jan 23, 2025

Choose a reason for hiding this comment

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

Thanks! Have you checked the godbolt link I added to the PR description? Alignment does not appear to increase in practice.
Also, IIUC the snippets you link use an explicit alignment parameter (i.e. aligned_storage<X, Y> not aligned_storage<X>), so they don't apply here. Even in the official guidance from the c++ committee there's no suggested replacement for aligned_storage<X> (see "Suggested replacement" section).
https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p1413r3.pdf

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for those details! With your change though, in practice there is a difference. See this: https://godbolt.org/z/hjGc8dqM9
Just switch which inplace_function.h gets included at the top, from the current one to your updated one. Alignment increases with this update : /

layers/external/inplace_function.h Outdated Show resolved Hide resolved
These are deprecated in c++23.

Note: in the one instance where the default Align value was used
for std::aligned_storage<Len, Alignment> (i.e.
std::aligned_storage<Len>), this patch now adopts
alignof(std::max_align_t). According to [1] that should be
a behavior change, but in practice it doesn't seem to be when
using clang nor gcc. In any case, any change in the alignment
would be an increase, which is fine.

[1] https://en.cppreference.com/w/cpp/types/aligned_storage
[2] https://godbolt.org/z/K5xGcxhjh
@ci-tester-lunarg
Copy link
Collaborator

Author victorvianna not on autobuild list. Waiting for curator authorization before starting CI build.

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.

3 participants