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

Turn format string issues into compile-time errors #9310

Merged
merged 1 commit into from Dec 5, 2020

Conversation

leoetlino
Copy link
Member

If the compiler can detect an issue with a format string at compile
time, then we should take advantage of that and turn the issue into a
hard compile-time error as such problems almost always lead to UB.

This helps with catching logging or assertion messages that have been
converted over to fmt but are still using the old, non-fmt variants
of the logging macros.

This commit also fixes all incorrect usages that I could find.

(Rebased version of #9284 which GitHub stopped updating for some reason)

If the compiler can detect an issue with a format string at compile
time, then we should take advantage of that and turn the issue into a
hard compile-time error as such problems almost always lead to UB.

This helps with catching logging or assertion messages that have been
converted over to fmt but are still using the old, non-fmt variants
of the logging macros.

This commit also fixes all incorrect usages that I could find.
# Format string issues that the compiler can detect should be compile time errors.
check_cxx_compiler_flag(-Wformat HAS_FORMAT_WARNING)
if (HAS_FORMAT_WARNING)
check_and_add_flag(FORMAT_WARNING_TO_ERROR -Werror=format)
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't been able to find an equivalent for VS unfortunately

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, MSVC seems to have format checks too, but only for some standard functions such as printf... :/

Copy link
Contributor

@iwubcode iwubcode left a comment

Choose a reason for hiding this comment

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

LGTM

@leoetlino leoetlino merged commit c8cb330 into dolphin-emu:master Dec 5, 2020
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants