[PATCH] D132568: [clang][Sema] check default argument promotions for printf

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGe3bd67eddf65: [clang][Sema] check default argument promotions for printf (authored by inclyc). Changed prior to commit: https://reviews.llvm.org/D

[PATCH] D132568: [clang][Sema] check default argument promotions for printf

inclyc updated this revision to Diff 457153. inclyc added a comment. Update comments according to review feedback Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132568/new/ https://reviews.llvm.org/D132568 Files: clang/docs/ReleaseNotes.rst cla

[PATCH] D132568: [clang][Sema] check default argument promotions for printf

efriedma added inline comments. Comment at: clang/docs/ReleaseNotes.rst:141 +- Implemented `WG14 N2562 `_. + Clang will now consider default argument promotions in printf, and remove unnecessary warnings. --

[PATCH] D132568: [clang][Sema] check default argument promotions for printf

aaron.ballman accepted this revision. aaron.ballman added a comment. Basically LGTM as well, just some nits about comments and a small fix to the c status page. In D132568#3753512 , @inclyc wrote: > Currently this patch has not fully implemented `wchar_

[PATCH] D132568: [clang][Sema] check default argument promotions for printf

nickdesaulniers accepted this revision. nickdesaulniers added a comment. This revision is now accepted and ready to land. @aaron.ballman should review+accept this before you land it, but I'm satisfied with the result. Thank you @inclyc for working on this! Repository: rG LLVM Github Monorepo

[PATCH] D132568: [clang][Sema] check default argument promotions for printf

inclyc updated this revision to Diff 456111. inclyc added a comment. Delete extra newline. Currently this patch has not fully implemented `wchar_t` related support, this type seems to be even platform dependent in C language, if possible, maybe we can consider support in subsequent patches? Rep

[PATCH] D132568: [clang][Sema] check default argument promotions for printf

inclyc updated this revision to Diff 455898. inclyc added a comment. Update comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132568/new/ https://reviews.llvm.org/D132568 Files: clang/docs/ReleaseNotes.rst clang/include/clang/AST/FormatSt

[PATCH] D132568: [clang][Sema] check default argument promotions for printf

inclyc added inline comments. Comment at: clang/lib/AST/FormatString.cpp:367 +case BuiltinType::Char_U: +case BuiltinType::Bool: + return Match; aaron.ballman wrote: > inclyc wrote: > > aaron.ballman wrote: > > > Isn't this a match promot

[PATCH] D132568: [clang][Sema] check default argument promotions for printf

inclyc updated this revision to Diff 455871. inclyc added a comment. Add LangOpts so we keep the Obj-C behavior of clang just as-is. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132568/new/ https://reviews.llvm.org/D132568 Files: clang/docs/Rel

[PATCH] D132568: [clang][Sema] check default argument promotions for printf

inclyc updated this revision to Diff 455793. inclyc added a comment. Trim whitespace in format.mm Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132568/new/ https://reviews.llvm.org/D132568 Files: clang/docs/ReleaseNotes.rst clang/include/clang

[PATCH] D132568: [clang][Sema] check default argument promotions for printf

dexonsmith added a subscriber: arphaman. dexonsmith added inline comments. Comment at: clang/test/FixIt/format.m:195-208 - NSLog(@"%C", 0x260300); // expected-warning{{format specifies type 'unichar' (aka 'unsigned short') but the argument has type 'int'}} - // CHECK-NOT: fix

[PATCH] D132568: [clang][Sema] check default argument promotions for printf

inclyc added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:10127-10129 +if (ImplicitMatch != ArgType::NoMatchPromotionTypeConfusion && +ImplicitMatch != ArgType::NoMatchTypeConfusion && +!IsCharacterLiteralInt) nickdesaulniers w

[PATCH] D132568: [clang][Sema] check default argument promotions for printf

inclyc updated this revision to Diff 455671. inclyc added a comment. Address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132568/new/ https://reviews.llvm.org/D132568 Files: clang/docs/ReleaseNotes.rst clang/include/clang/AST/FormatS

[PATCH] D132568: [clang][Sema] check default argument promotions for printf

nickdesaulniers added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:10133-10135 + if (ImplicitMatch == ArgType::NoMatchPedantic || + ImplicitMatch == ArgType::NoMatchTypeConfusion) +Match = ImplicitMatch; nickdesaulniers wrote: > Similarl

[PATCH] D132568: [clang][Sema] check default argument promotions for printf

nickdesaulniers added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:10133-10135 + if (ImplicitMatch == ArgType::NoMatchPedantic || + ImplicitMatch == ArgType::NoMatchTypeConfusion) +Match = ImplicitMatch; Similarly, if `ImplicitMatch` can

[PATCH] D132568: [clang][Sema] check default argument promotions for printf

nickdesaulniers added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:10127-10129 +if (ImplicitMatch != ArgType::NoMatchPromotionTypeConfusion && +ImplicitMatch != ArgType::NoMatchTypeConfusion && +!IsCharacterLiteralInt) There's

[PATCH] D132568: [clang][Sema] check default argument promotions for printf

nickdesaulniers added inline comments. Comment at: clang/lib/AST/FormatString.cpp:401 + if (const auto *BT = argTy->getAs()) { +if (!Ptr) { + switch (BT->getKind()) { inclyc wrote: > nickdesaulniers wrote: > > aaron.ballman wrote: > > > It's

[PATCH] D132568: [clang][Sema] check default argument promotions for printf

inclyc updated this revision to Diff 455645. inclyc added a comment. Remove wchar tests. These tests may need to be re-added after we have implemented `wchar_t` checks in C Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132568/new/ https://reviews

[PATCH] D132568: [clang][Sema] check default argument promotions for printf

inclyc added inline comments. Comment at: clang/lib/AST/FormatString.cpp:401 + if (const auto *BT = argTy->getAs()) { +if (!Ptr) { + switch (BT->getKind()) { nickdesaulniers wrote: > aaron.ballman wrote: > > It's a bit strange that we have t

[PATCH] D132568: [clang][Sema] check default argument promotions for printf

nickdesaulniers added a comment. In D132568#3747971 , @inclyc wrote: >> Do we want to encode that in `test_promotion` in >> `clang/test/Sema/format-strings.c`? Seems like tests on shorts are missing. > > Tests for short and char "incompatibility" could b

[PATCH] D132568: [clang][Sema] check default argument promotions for printf

inclyc updated this revision to Diff 455626. inclyc added a comment. Address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132568/new/ https://reviews.llvm.org/D132568 Files: clang/docs/ReleaseNotes.rst clang/include/clang/AST/FormatS

[PATCH] D132568: [clang][Sema] check default argument promotions for printf

aaron.ballman added inline comments. Comment at: clang/lib/AST/FormatString.cpp:367 +case BuiltinType::Char_U: +case BuiltinType::Bool: + return Match; inclyc wrote: > aaron.ballman wrote: > > Isn't this a match promotion as well? e.g. `p

[PATCH] D132568: [clang][Sema] check default argument promotions for printf

aaron.ballman added inline comments. Comment at: clang/lib/AST/FormatString.cpp:422 + case BuiltinType::Bool: +// Don't warn printf("%hd", [char]) +// https://reviews.llvm.org/D66186 inclyc wrote: > aaron.ballman wrote: > > The co

[PATCH] D132568: [clang][Sema] check default argument promotions for printf

inclyc marked 2 inline comments as done. inclyc added inline comments. Comment at: clang/lib/AST/FormatString.cpp:367 +case BuiltinType::Char_U: +case BuiltinType::Bool: + return Match; aaron.ballman wrote: > Isn't this a match promotion

[PATCH] D132568: [clang][Sema] check default argument promotions for printf

inclyc added inline comments. Comment at: clang/lib/AST/FormatString.cpp:422 + case BuiltinType::Bool: +// Don't warn printf("%hd", [char]) +// https://reviews.llvm.org/D66186 aaron.ballman wrote: > The comment is talking about pa

[PATCH] D132568: [clang][Sema] check default argument promotions for printf

aaron.ballman added inline comments. Comment at: clang/test/FixIt/format.m:40 -void test_object_correction (id x) { +void test_object_correction (id x) { NSLog(@"%d", x); // expected-warning{{format specifies type 'int' but the argument has type 'id'}} in

[PATCH] D132568: [clang][Sema] check default argument promotions for printf

inclyc added inline comments. Comment at: clang/test/FixIt/format.m:40 -void test_object_correction (id x) { +void test_object_correction (id x) { NSLog(@"%d", x); // expected-warning{{format specifies type 'int' but the argument has type 'id'}} aaron.bal

[PATCH] D132568: [clang][Sema] check default argument promotions for printf

aaron.ballman added subscribers: dexonsmith, rjmccall. aaron.ballman added a comment. In D132568#3747598 , @nickdesaulniers wrote: > In D132568#3746551 , @aaron.ballman > wrote: > >> Thank you for the patch, but

[PATCH] D132568: [clang][Sema] check default argument promotions for printf

inclyc updated this revision to Diff 455485. inclyc added a comment. Fix CI issue Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132568/new/ https://reviews.llvm.org/D132568 Files: clang/docs/ReleaseNotes.rst clang/include/clang/AST/FormatStrin

[PATCH] D132568: [clang][Sema] check default argument promotions for printf

inclyc updated this revision to Diff 455477. inclyc added a comment. comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132568/new/ https://reviews.llvm.org/D132568 Files: clang/docs/ReleaseNotes.rst clang/include/clang/AST/FormatString.h

[PATCH] D132568: [clang][Sema] check default argument promotions for printf

inclyc updated this revision to Diff 455475. inclyc added a comment. Comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132568/new/ https://reviews.llvm.org/D132568 Files: clang/docs/ReleaseNotes.rst clang/include/clang/AST/FormatString.h

[PATCH] D132568: [clang][Sema] check default argument promotions for printf

inclyc updated this revision to Diff 455474. inclyc added a comment. Add tests for character literals I've noticed that Linus mentioned the following code triggered warning by clang. (With a little modifications shown below) Link: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.g

[PATCH] D132568: [clang][Sema] check default argument promotions for printf

inclyc updated this revision to Diff 455459. inclyc added a comment. rm {} Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132568/new/ https://reviews.llvm.org/D132568 Files: clang/docs/ReleaseNotes.rst clang/include/clang/AST/FormatString.h c

[PATCH] D132568: [clang][Sema] check default argument promotions for printf

inclyc updated this revision to Diff 455458. inclyc added a comment. Address comments & more tests & docs Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132568/new/ https://reviews.llvm.org/D132568 Files: clang/docs/ReleaseNotes.rst clang/inclu

[PATCH] D132568: [clang][Sema] check default argument promotions for printf

inclyc added a comment. > Do we want to encode that in `test_promotion` in > `clang/test/Sema/format-strings.c`? Seems like tests on shorts are missing. Tests for short and char "incompatibility" could be found elsewhere in this file. format-strings.c void should_understand_small_integers(v

[PATCH] D132568: [clang][Sema] check default argument promotions for printf

nickdesaulniers added a comment. In D132568#3746551 , @aaron.ballman wrote: > Thank you for the patch, but it'd be really helpful to me as a reviewer if > you and @nickdesaulniers could coordinate so there's only one patch trying to > address #57102 in

[PATCH] D132568: [clang][Sema] check default argument promotions for printf

aaron.ballman added a subscriber: nickdesaulniers. aaron.ballman added a comment. Thank you for the patch, but it'd be really helpful to me as a reviewer if you and @nickdesaulniers could coordinate so there's only one patch trying to address #57102 instead of two competing patches (I'm happy to

[PATCH] D132568: [clang][Sema] check default argument promotions for printf

inclyc created this revision. inclyc added a reviewer: aaron.ballman. Herald added a subscriber: emaste. Herald added a project: All. inclyc published this revision for review. inclyc added a comment. Herald added a project: clang. Herald added a subscriber: cfe-commits. https://reviews.llvm.org/D