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

silence warning C4100 on MSVC 2019 when exceptions are disabled #2397

Merged
merged 1 commit into from
Jul 2, 2021

Conversation

mattiasljungstrom
Copy link
Contributor

Removes the following warnings under MSVC 2019 (16.10.2), with W4 enabled and when exceptions are disabled. (/W4 -D_HAS_EXCEPTIONS=0)

fmt\include\fmt\format.h(810,44): warning C4100: 'message': unreferenced formal parameter
fmt\include\fmt/format-inl.h(2553,59): warning C4100: 'message': unreferenced formal parameter

@mattiasljungstrom mattiasljungstrom changed the title silence warning C4100 on MSVC when exceptions are disabled silence warning C4100 on MSVC 2019 when exceptions are disabled Jun 28, 2021
@vitaut
Copy link
Contributor

vitaut commented Jun 29, 2021

This is strange because FMT_THROW always uses the argument:

fmt/include/fmt/format.h

Lines 91 to 100 in a3f762c

# define FMT_THROW(x) detail::do_throw(x)
# else
# define FMT_THROW(x) throw x
# endif
# else
# define FMT_THROW(x) \
do { \
FMT_ASSERT(false, (x).what()); \
} while (false)
# endif

Do you have a godbolt repro?

@mattiasljungstrom
Copy link
Contributor Author

I believe that in release mode the FMT_ASSERT is declared ((void)0), which means that the compiler removes everything in the FMT_THROW(x) macro. (Given that exceptions are disabled.)

@vitaut
Copy link
Contributor

vitaut commented Jun 29, 2021

Right. In this case it should be fixed in the FMT_ASSERT macro, not in individual call sites.

@phprus
Copy link
Contributor

phprus commented Jun 29, 2021

@vitaut, maybe replace

# define FMT_ASSERT(condition, message) ((void)0)

to:

FMT_INLINE void assert_ignore(bool, const char*) {}
#    define FMT_ASSERT(condition, message) ::fmt::detail::assert_ignore((condition), (message))

?

@mattiasljungstrom, please check this solution.

@mattiasljungstrom
Copy link
Contributor Author

mattiasljungstrom commented Jun 29, 2021

@vitaut makes sense!

This works for my case:

#    define FMT_ASSERT(condition, message) ((void)message) 

Or this works for MSVC (note constexpr needed), haven't tried other compilers:

FMT_INLINE constexpr void assert_ignore(bool, const char*) {}
#    define FMT_ASSERT(condition, message) ::fmt::detail::assert_ignore((condition), (message))

@mattiasljungstrom
Copy link
Contributor Author

I've updated the PR to use the assert_ignore() solution for FMT_ASSERT.

@vitaut
Copy link
Contributor

vitaut commented Jul 2, 2021

Can we use the newly added ignore_unused

template <class T> void ignore_unused(const T&) {}

instead of introducing a new function?

@mattiasljungstrom
Copy link
Contributor Author

Sure! I've changed the code to use the ::fmt::ignore_unused(), but I had to add FMT_CONSTEXPR since the assert is sometimes used in constexpr functions.

@phprus
Copy link
Contributor

phprus commented Jul 2, 2021

Maybe rewrite ignore_unused function as:

template <class ...Ts> FMT_CONSTEXPR void ignore_unused(const Ts& ...) {}

and remove do...while block?

@vitaut
Copy link
Contributor

vitaut commented Jul 2, 2021

Maybe rewrite ignore_unused function as

Was about to suggest the same =)

@mattiasljungstrom
Copy link
Contributor Author

updated code with @phprus suggestions.

@vitaut vitaut merged commit 54014e4 into fmtlib:master Jul 2, 2021
@vitaut
Copy link
Contributor

vitaut commented Jul 2, 2021

Thank you!

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