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

2019-08-23 Thread Nathan Huckleberry via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rL369791: [Sema] Dont 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)

2019-08-23 Thread Roman Lebedev via Phabricator via cfe-commits
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)

2019-08-23 Thread Nathan Huckleberry via Phabricator via cfe-commits
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

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

2019-08-23 Thread Roman Lebedev via Phabricator via cfe-commits
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 ==

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

2019-08-23 Thread Nathan Huckleberry via Phabricator via cfe-commits
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

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

2019-08-23 Thread Roman Lebedev via Phabricator via cfe-commits
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

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

2019-08-23 Thread Aaron Ballman via Phabricator via cfe-commits
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)

2019-08-23 Thread Roman Lebedev via Phabricator via cfe-commits
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) +

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

2019-08-22 Thread Nick Desaulniers via Phabricator via cfe-commits
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)

2019-08-22 Thread Nathan Huckleberry via Phabricator via cfe-commits
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:

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

2019-08-22 Thread Nick Desaulniers via Phabricator via cfe-commits
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)

2019-08-22 Thread Nathan Huckleberry via Phabricator via cfe-commits
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:

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

2019-08-22 Thread Roman Lebedev via Phabricator via cfe-commits
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

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

2019-08-22 Thread Nathan Huckleberry via Phabricator via cfe-commits
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

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

2019-08-22 Thread Nathan Huckleberry via Phabricator via cfe-commits
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

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

2019-08-22 Thread Nathan Huckleberry via Phabricator via cfe-commits
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

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

2019-08-22 Thread Nathan Huckleberry via Phabricator via cfe-commits
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

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

2019-08-22 Thread Aaron Ballman via Phabricator via cfe-commits
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

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

2019-08-22 Thread Roman Lebedev via Phabricator via cfe-commits
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 ==

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

2019-08-22 Thread Nathan Huckleberry via Phabricator via cfe-commits
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:

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

2019-08-22 Thread Nick Desaulniers via Phabricator via cfe-commits
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

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

2019-08-22 Thread Aaron Ballman via Phabricator via cfe-commits
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

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

2019-08-22 Thread Roman Lebedev via Phabricator via cfe-commits
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 ==

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

2019-08-22 Thread Nathan Huckleberry via Phabricator via cfe-commits
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

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

2019-08-22 Thread Nathan Huckleberry via Phabricator via cfe-commits
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:

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

2019-08-16 Thread Aaron Ballman via Phabricator via cfe-commits
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 >

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

2019-08-15 Thread Nathan Huckleberry via Phabricator via cfe-commits
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

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

2019-08-15 Thread Roman Lebedev via Phabricator via cfe-commits
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/

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

2019-08-15 Thread Aaron Ballman via Phabricator via cfe-commits
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 > >

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

2019-08-15 Thread Roman Lebedev via Phabricator via cfe-commits
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

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

2019-08-15 Thread Nathan Huckleberry via Phabricator via cfe-commits
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

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

2019-08-14 Thread Aaron Ballman via Phabricator via cfe-commits
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