Skip to content

Make the build work with -Wformat=2#2088

Merged
dmah42 merged 2 commits intogoogle:mainfrom
davidben:w-format-2
Dec 8, 2025
Merged

Make the build work with -Wformat=2#2088
dmah42 merged 2 commits intogoogle:mainfrom
davidben:w-format-2

Conversation

@davidben
Copy link
Contributor

@davidben davidben commented Dec 7, 2025

The -Wformat=2 macro requires every format string either be a string literal, or a parameter that was tagged with the format attribute. The upshot is that it expects every printf function to be marked, or you get warnings like this:

src/console_reporter.cc:104:23: error: format string is not a string literal [-Werror,-Wformat-nonliteral]
  104 |   out << FormatString(fmt, args);
      |                       ^~~

Marking such things is generally worthwhile since it turns on error-checking within the library, so fill in the missing ones.

Tested with:

bazelisk build --copt=-Werror --copt=-Wformat=2 :all

The -Wformat=2 macro requires every format string either be a string
literal, or a parameter that was tagged with the format attribute. The
upshot is that it expects every printf function to be marked, or you get
warnings like this:

    src/console_reporter.cc:104:23: error: format string is not a string literal [-Werror,-Wformat-nonliteral]
      104 |   out << FormatString(fmt, args);
          |                       ^~~

Marking such things is generally worthwhile since it turns on
error-checking within the library, so fill in the missing ones.

Tested with:

    bazelisk build --copt=-Werror --copt=-Wformat=2 :all
@LebedevRI
Copy link
Collaborator

Should this diagnostic be enabled in CMake build then, otherwise things can regress back?

@davidben
Copy link
Contributor Author

davidben commented Dec 7, 2025

Figured I'd defer to the project maintainers on that. I noticed this because we set this diagnostic in BoringSSL and (for now, but I'm considering changing this) the way we set our diagnostics hits dependencies. But I think this PR makes sense to merge either way, so I figure that can be a separate decision.

@LebedevRI
Copy link
Collaborator

Figured I'd defer to the project maintainers on that.

That was the gist of my comment, yes. Aka, let's enable it or it will break again.

I noticed this because we set this diagnostic in BoringSSL and (for now, but I'm considering changing this) the way we set our diagnostics hits dependencies. But I think this PR makes sense to merge either way, so I figure that can be a separate decision.

@davidben
Copy link
Contributor Author

davidben commented Dec 8, 2025

Added to CMake and Bazel builds.

@dmah42 dmah42 merged commit 009f05c into google:main Dec 8, 2025
83 of 84 checks passed
@dmah42
Copy link
Member

dmah42 commented Dec 8, 2025

lovely. thank you.

@LebedevRI
Copy link
Collaborator

@dmah42 you beat me to it.
@davidben 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