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 clang -Wunused-member-function warnings with FMT_MAYBE_UNUSED. #1576

Merged
merged 1 commit into from
Mar 13, 2020

Conversation

refnum
Copy link
Contributor

@refnum refnum commented Mar 2, 2020

FMT_MAYBE_UNUSED currently expands to the [[maybe_unused]] attribute on C++17.

It could be extended to previous language versions with a compiler-specific attribute if required.

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! Looks good, just one minor comment inline.

@@ -132,6 +132,14 @@
# define FMT_NORETURN
#endif

#ifndef FMT_MAYBE_UNUSED
# if (__cplusplus >= 201703L)
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest replacing this check with FMT_HAS_CPP_ATTRIBUTE(maybe_unused).

Copy link
Contributor Author

@refnum refnum Mar 5, 2020

Choose a reason for hiding this comment

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

Updated to use FMT_HAS_CPP_ATTRIBUTE as requested.

However this fails the Travis C++14 build; although __has_cpp_attribute returns false it also triggers a warning that this attribute requires C++17.

That's a useful warning to have, to avoid accidental usage, so this could be something like __cplusplus >= 201703L && FMT_HAS_CPP_ATTRIBUTE(maybe_unused) as per FMT_FALLTHROUGH.

Perhaps a more general approach, to test both language version and compiler support within that version, could be:

#define FMT_HAS_CPP17_ATTRIBUTE(_attribute)  (__cplusplus >= 201703L && FMT_HAS_CPP_ATTRIBUTE(_attribute))

And then use that for FMT_MAYBE_UNUSED and FMT_FALLTHROUGH.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps a more general approach, to test both language version and compiler support within that version

Good idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, thanks - I've added a FMT_HAS_CPP14_ATTRIBUTE to cover FMT_DEPRECATED as well so all of the version+attribute checks now use this approach.

@@ -36,6 +36,9 @@
# define FMT_HAS_CPP_ATTRIBUTE(x) 0
#endif

#define FMT_HAS_CPP14_ATTRIBUTE(_attribute) (__cplusplus >= 201402L && 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.

Why underscore in _attribute?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I normally underscore macro parameters to help distinguish them from regular variable names.

That's not the normal fmt style though so I've removed them and run clang-format as requested.

@vitaut
Copy link
Contributor

vitaut commented Mar 13, 2020

LGTM but please apply clang-format to your changes. Thanks!

…guage-specific attributes.

FMT_DEPRECATED is now defined as FMT_HAS_CPP14_ATTRIBUTE(deprecated), as this attribute was introduced in C++14.

FMT_FALLTHROUGH is now defined as FMT_HAS_CPP17_ATTRIBUTE(fallthrough), as this attribute was introduced in C++17.

FMT_MAYBE_UNUSED is defined as FMT_HAS_CPP17_ATTRIBUTE(maybe_unused), as this attribute was introduced in C++17.

FMT_MAYBE_UNUSED has been applied to fix a couple of -Wunused-member-function warnings from clang.
@vitaut vitaut merged commit 02bfd8a into fmtlib:master Mar 13, 2020
@refnum refnum deleted the maybe_unused branch March 13, 2020 18:11
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