[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

This revision was automatically updated to reflect the committed changes. Closed by commit rL369791: [Sema] Don't warn on printf('%hd', [char]) (PR41467) (authored by Nathan-Huckleberry, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

lebedev.ri accepted this revision. lebedev.ri added a comment. This revision is now accepted and ready to land. LG, thank you. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66186/new/ https://reviews.llvm.org/D66186 _

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

Nathan-Huckleberry updated this revision to Diff 216893. Nathan-Huckleberry added a comment. - Add if without else Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66186/new/ https://reviews.llvm.org/D66186 Files: clang/lib/AST/FormatString.cpp c

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

lebedev.ri requested changes to this revision. lebedev.ri added inline comments. This revision now requires changes to proceed. Comment at: clang/lib/Sema/SemaChecking.cpp:8106 return true; +Pedantic = true; } `if (ImplicitMatch == analy

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

Nathan-Huckleberry updated this revision to Diff 216884. Nathan-Huckleberry added a comment. - Remove else Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66186/new/ https://reviews.llvm.org/D66186 Files: clang/lib/AST/FormatString.cpp clang/lib

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

lebedev.ri accepted this revision. lebedev.ri added a comment. This revision is now accepted and ready to land. In D66186#1642607 , @aaron.ballman wrote: > LGTM aside from the else after return nit. Repository: rG LLVM Github Monorepo CHANGES SINC

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

aaron.ballman accepted this revision. aaron.ballman added a comment. LGTM aside from the else after return nit. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66186/new/ https://reviews.llvm.org/D66186

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

lebedev.ri added a comment. Looks about right Comment at: clang/lib/Sema/SemaChecking.cpp:8105-8106 +if (ImplicitMatch == analyze_printf::ArgType::Match) return true; +else if (ImplicitMatch == analyze_printf::ArgType::NoMatchPedantic) + Peda

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

nickdesaulniers accepted this revision. nickdesaulniers added a comment. Thanks for being responsive to all this code review! 💃🏽 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66186/new/ https://reviews.llvm.org/D66186 ___

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

Nathan-Huckleberry updated this revision to Diff 216739. Nathan-Huckleberry added a comment. - Simplify logic for readability Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66186/new/ https://reviews.llvm.org/D66186 Files: clang/lib/AST/FormatStr

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

nickdesaulniers added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:8107 +return true; + Pedantic |= ImplicitMatch == analyze_printf::ArgType::NoMatchPedantic; +} At this point `ImplicitMatch` can only have the value

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

Nathan-Huckleberry updated this revision to Diff 216730. Nathan-Huckleberry added a comment. - Fix broken logic from previous revision Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66186/new/ https://reviews.llvm.org/D66186 Files: clang/lib/AST/

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

lebedev.ri requested changes to this revision. lebedev.ri added a comment. This revision now requires changes to proceed. Please can you explain why the snippet i posted in-line does not work for you? Comment at: clang/lib/Sema/SemaChecking.cpp:8105-8107 + if (Implicit

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

Nathan-Huckleberry updated this revision to Diff 216729. Nathan-Huckleberry added a comment. - Remove else and |= Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66186/new/ https://reviews.llvm.org/D66186 Files: clang/lib/AST/FormatString.cpp cl

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

Nathan-Huckleberry marked an inline comment as done. Nathan-Huckleberry added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:8100-8107 +// All further checking is done on the subexpression +Match = AT.matchesType(S.Context, ExprTy); +if (Matc

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

Nathan-Huckleberry marked an inline comment as done. Nathan-Huckleberry added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:8100-8107 +// All further checking is done on the subexpression +Match = AT.matchesType(S.Context, ExprTy); +if (Matc

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

Nathan-Huckleberry marked an inline comment as done. Nathan-Huckleberry added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:8100-8107 +// All further checking is done on the subexpression +Match = AT.matchesType(S.Context, ExprTy); +if (Matc

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

aaron.ballman added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:8106 + if (ImplicitMatch == analyze_printf::ArgType::NoMatchPedantic) +Pedantic |= true; + else I don't think this needs to use `|= true`. If `Pedantic`

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

lebedev.ri added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:8100-8107 +// All further checking is done on the subexpression +Match = AT.matchesType(S.Context, ExprTy); +if (Match) { + if (Match == analyze_printf::ArgType::NoMatch

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

Nathan-Huckleberry updated this revision to Diff 216713. Nathan-Huckleberry added a comment. - Add variable for implicit match and fix comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66186/new/ https://reviews.llvm.org/D66186 Files: clang

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

nickdesaulniers added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:8101 +// All further checking is done on the subexpression +Match = AT.matchesType(S.Context, ExprTy); +if (Match) { Maybe leave the top level Match const a

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

aaron.ballman added inline comments. Comment at: clang/test/Sema/format-strings-enum-fixed-type.cpp:82 // This is not correct but it is safe. We warn because '%hd' shows intent. + printf("%hd", input);// no-warning aaron.ballman wrote: > This commen

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

lebedev.ri added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:8080-8083 + analyze_printf::ArgType::MatchKind Match = AT.matchesType(S.Context, ExprTy); bool Pedantic = Match == analyze_printf::ArgType::NoMatchPedantic; if (Match == analyze_printf::ArgType::

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

Nathan-Huckleberry updated this revision to Diff 216702. Nathan-Huckleberry added a comment. - Cleanup test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66186/new/ https://reviews.llvm.org/D66186 Files: clang/lib/AST/FormatString.cpp clang/li

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

Nathan-Huckleberry updated this revision to Diff 216699. Nathan-Huckleberry added a comment. - Warn when -Wformat-pedantic is set Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66186/new/ https://reviews.llvm.org/D66186 Files: clang/lib/AST/Forma

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

aaron.ballman requested changes to this revision. aaron.ballman added a comment. In D66186#1632440 , @Nathan-Huckleberry wrote: > As far as I can tell this case was just overlooked. The original commit > adding this change > https://reviews.llvm.org/rG0

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

Nathan-Huckleberry added a comment. As far as I can tell this case was just overlooked. The original commit adding this change https://reviews.llvm.org/rG0208793e41018ac168412a3da8b2fba70aba9716 only allows chars to int and chars to chars. Another commit ignores typing of chars https://reviews.

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

lebedev.ri requested changes to this revision. lebedev.ri added a comment. This revision now requires changes to proceed. (just want to mark it as "unanswered questions") Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66186/new/ https://reviews.llvm

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

aaron.ballman added a comment. In D66186#1631921 , @Nathan-Huckleberry wrote: > In D66186#1630427 , @aaron.ballman > wrote: > > > There was a request in the linked bug for some code archaeology to see why > > thi

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

lebedev.ri added a comment. In D66186#1631921 , @Nathan-Huckleberry wrote: > In D66186#1630427 , @aaron.ballman > wrote: > > > There was a request in the linked bug for some code archaeology to see why > > this b

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

Nathan-Huckleberry added a comment. In D66186#1630427 , @aaron.ballman wrote: > There was a request in the linked bug for some code archaeology to see why > this behavior exists in the first place. What were the results of that? I'm > not opposed to the

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

aaron.ballman added a comment. There was a request in the linked bug for some code archaeology to see why this behavior exists in the first place. What were the results of that? I'm not opposed to the patch, but I would like to understand why it behaves the way it does. I could imagine "confus