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

Give basic_memory_buffer allocator [[no_unique_address]] #3485

Merged
merged 1 commit into from Jun 13, 2023

Conversation

Minty-Meeo
Copy link
Contributor

This allows stateless allocators to take up no space while still avoiding the empty base class optimization.

@Minty-Meeo
Copy link
Contributor Author

Minty-Meeo commented Jun 12, 2023

Following the advice from https://devblogs.microsoft.com/cppblog/msvc-cpp20-and-the-std-cpp20-switch/ on how to use [[msvc::no_unique_address]]. Attempting to use it in standards prior to C++20 works, but emits a warning, so I opted to only enable this when FMT_CPLUSPLUS >= 202002L. Alternatively, a pragma warning disable for MSVC can be put around the allocator for basic_memory_buffer.

Copy link
Contributor

@vitaut vitaut 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 PR

Comment on lines 160 to 169
#ifndef FMT_NO_UNIQUE_ADDRESS
# if FMT_HAS_CPP20_ATTRIBUTE(no_unique_address)
# define FMT_NO_UNIQUE_ADDRESS [[no_unique_address]]
# elif FMT_MSC_VERSION >= 1929 && FMT_CPLUSPLUS >= 202002L // VS2019 v16.10 and later
# define FMT_NO_UNIQUE_ADDRESS [[msvc::no_unique_address]]
# else
# define FMT_NO_UNIQUE_ADDRESS
# endif
#endif

Copy link
Contributor

Choose a reason for hiding this comment

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

Please move to format.h where it is used.

Comment on lines 90 to 91
#define FMT_HAS_CPP20_ATTRIBUTE(attribute) \
(FMT_CPLUSPLUS >= 202002L && FMT_HAS_CPP_ATTRIBUTE(attribute))
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need this for a single check esp. since MSVC requires special handling. Let's just inline in the call site.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it not be better to create this feature now, in the event that the other C++20 attributes (likely, unlikely) are desired in the future?

Copy link
Contributor

Choose a reason for hiding this comment

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

There is little value in these variations of FMT_HAS_CPP_ATTRIBUTE so let's just inline.

This allows stateless allocators to take up no space while still avoiding the empty base class optimization.
@vitaut vitaut merged commit c86fe0b into fmtlib:master Jun 13, 2023
40 checks passed
@vitaut
Copy link
Contributor

vitaut commented Jun 13, 2023

Thanks

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.

None yet

2 participants