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

Fix FMT_NO_UNIQUE_ADDRESS warning with clang-cl. #3600

Merged
merged 1 commit into from
Aug 20, 2023

Conversation

lukester1975
Copy link
Contributor

@lukester1975 lukester1975 commented Aug 20, 2023

Hi

clang-cl is generating a warning with the no_unique_address change from c86fe0b: warning : unknown attribute 'no_unique_address' ignored [-Wunknown-attributes]

Thread on clang-cl and no_unique_address: https://reviews.llvm.org/D110485

Here's an additional check check for clang-cl (plus use __has_cpp_attribute(msvc::no_unique_address) to cope in case clang-cl adds support; cl returns true since 19.31).

Thanks

@lukester1975 lukester1975 force-pushed the fix-no-unique-address branch from 1e7e972 to 96bd516 Compare August 20, 2023 13:47
@@ -83,7 +83,9 @@
# if FMT_CPLUSPLUS >= 202002L
# if FMT_HAS_CPP_ATTRIBUTE(no_unique_address)
# define FMT_NO_UNIQUE_ADDRESS [[no_unique_address]]
# elif FMT_MSC_VERSION >= 1929 // VS2019 v16.10 and later
// VS2019 v16.10 and later except clang-cl (https://reviews.llvm.org/D110485)
# elif __has_cpp_attribute(msvc::no_unique_address) || \
Copy link
Contributor

Choose a reason for hiding this comment

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

__has_cpp_attribute -> FMT_HAS_CPP_ATTRIBUTE

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I don't think we need an attribute check since the branch below only handles specific compiler (versions).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, FMT_HAS_CPP_ATTRIBUTE done, missed that.

The reason I put the msvc::no_unique_address attr check is to future proof if support gets added to clang-cl, rather than always not setting.

I suppose if clang-cl waits to enable the standard no_unique_address until after cl does and never bothers with the msvc attr it'll be moot, but this way at least it won't need revising either way... Or at least that was the intention!

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 should be adding support for some not-standard attribute that may or may not be supported in the future. Let's remove the attribute check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK sure, done.

Seems a shame to miss out on the size optimization if it does get added, though! Not that I've looked at where it's used in fmtlib 😀

@lukester1975 lukester1975 force-pushed the fix-no-unique-address branch 2 times, most recently from b5da2db to 50afbbb Compare August 20, 2023 15:20
@lukester1975 lukester1975 force-pushed the fix-no-unique-address branch from 50afbbb to 8ce4b15 Compare August 20, 2023 16:13
@vitaut vitaut merged commit cc077a5 into fmtlib:master Aug 20, 2023
@vitaut
Copy link
Contributor

vitaut commented Aug 20, 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.

2 participants