Page MenuHomePhabricator

[Sema] Split out -Wformat-type-confusion from -Wformat-pedantic
ClosedPublic

Authored by erik.pilkington on Sep 19 2019, 1:12 PM.

Details

Summary

The warnings now in -Wformat-type-confusion don't align with how we interpret 'pedantic' in clang, and don't belong in -pedantic.

Suggested by Aaron here: D66856

Thanks for taking a look!
Erik

Diff Detail

Repository
rL LLVM

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptSep 19 2019, 1:12 PM

Ping!

Sorry for the delayed review -- thank you for working on clearing this up!

clang/include/clang/AST/FormatString.h
259 ↗(On Diff #220902)

Can you add: For instance, "%d" and _Bool.

clang/lib/Sema/SemaChecking.cpp
8165 ↗(On Diff #220902)

How about:

if (ImplicitMatch == ArgType::NoMatchPedantic ||
    ImplicitMatch == ArgType::NoMatchTypeConfusion)
  Match = ImplicitMatch;
clang/test/Sema/format-strings-pedantic.c
1 ↗(On Diff #220902)

Are we losing test coverage for -Wformat-pedantic, or do we have other tests covering that elsewhere? I would have expected this test file's contents to exercise pedantic cases.

clang/test/Sema/format-type-confusion.c
13 ↗(On Diff #220902)

Just double-checking, but the reason we don't diagnose the %c here is because of -Wno-format?

erik.pilkington marked 6 inline comments as done.

Address review comments.

clang/test/Sema/format-strings-pedantic.c
1 ↗(On Diff #220902)

The only warning that was in this file is now under -Wformat-type-confusion. New patch adds a test for the printf("%p", (int*)0); thing, which was otherwise untested.

clang/test/Sema/format-type-confusion.c
13 ↗(On Diff #220902)

Yup, exactly. I just wanted to test -Wformat-type-confusion alone in this file.

aaron.ballman accepted this revision.Oct 4 2019, 11:38 AM

LGTM, thank you for this!

clang/test/Sema/format-strings-pedantic.c
1 ↗(On Diff #220902)

Thank you for adding that!

clang/test/Sema/format-type-confusion.c
13 ↗(On Diff #220902)

Awesome, thank you for the verification!

This revision is now accepted and ready to land.Oct 4 2019, 11:38 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 4 2019, 12:23 PM