nickdesaulniers added a comment. In D132568#3746551 <https://reviews.llvm.org/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 instead of two competing patches (I'm happy to review > whichever patch comes out). From a quick look over the test case changes, > this patch looks like it's more in line with how I'd expect to resolve that > issue compared to D132266 <https://reviews.llvm.org/D132266>. The addition to clang/test/Sema/format-strings.c looks like it will resolve Linus' complaints in https://github.com/llvm/llvm-project/issues/57102; I'm happy to abandon D132266 <https://reviews.llvm.org/D132266> in preference of this. > However, the usage of %hhd corresponding to short is relatively rare, and it > is more likely to be a misuse. Do we want to encode that in `test_promotion` in `clang/test/Sema/format-strings.c`? Seems like tests on shorts are missing. ================ Comment at: clang/lib/AST/FormatString.cpp:359 - if (const BuiltinType *BT = argTy->getAs<BuiltinType>()) + if (const BuiltinType *BT = argTy->getAs<BuiltinType>()) { switch (BT->getKind()) { ---------------- might as well make this `auto` while you're here. ================ Comment at: clang/lib/AST/FormatString.cpp:408-410 + T == C.ShortTy || T == C.UnsignedShortTy) { + return MatchPromotion; + } ---------------- `{}` aren't necessary ================ Comment at: clang/lib/AST/FormatString.cpp:414-417 + if (T == C.SignedCharTy || T == C.UnsignedCharTy || T == C.IntTy || + T == C.UnsignedIntTy) { + return NoMatchPromotionTypeConfusion; + } ---------------- `{}` aren't necessary ================ Comment at: clang/lib/AST/FormatString.cpp:426-428 + if (T == C.IntTy || T == C.UnsignedIntTy) { + return NoMatchPromotionTypeConfusion; + } ---------------- `{}` aren't necessary ================ Comment at: clang/lib/Sema/SemaChecking.cpp:10084-10085 - analyze_printf::ArgType::MatchKind Match = AT.matchesType(S.Context, ExprTy); - if (Match == analyze_printf::ArgType::Match) + ArgType::MatchKind ImplicitMatch = ArgType::NoMatch, + Match = AT.matchesType(S.Context, ExprTy); + if (Match == ArgType::Match) ---------------- I think it would be slightly more readable to make these two assignments two dedicated statements. ================ Comment at: clang/test/Sema/format-strings-scanf.c:271-272 + scanf("%llx", &i); // expected-warning{{format specifies type 'unsigned long long *' but the argument has type 'int *'}} + scanf("%hf", // expected-warning{{length modifier 'h' results in undefined behavior or no effect with 'f' conversion specifier}} + &sc); // Is this a clang bug ? + scanf("%s", i); // expected-warning{{format specifies type 'char *' but the argument has type 'int'}} ---------------- Hmm... ================ Comment at: clang/test/Sema/format-strings-scanf.c:274 + scanf("%s", i); // expected-warning{{format specifies type 'char *' but the argument has type 'int'}} + +} ---------------- delete empty newline Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132568/new/ https://reviews.llvm.org/D132568 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
