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 <https://reviews.llvm.org/D132568#3753512>, @inclyc wrote: > 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? I think that's reasonable. ================ Comment at: clang/lib/AST/FormatString.cpp:360 + if (const auto *BT = argTy->getAs<BuiltinType>()) { + // the types are perfectly matched? switch (BT->getKind()) { ---------------- ================ Comment at: clang/lib/AST/FormatString.cpp:371 + } + // "partially matched" because of promotions? + if (!Ptr) { ---------------- ================ Comment at: clang/lib/AST/FormatString.cpp:404 + if (const auto *BT = argTy->getAs<BuiltinType>()) { + // check if the only difference between them is signed vs unsigned + // if true, we consider they are compatible. ---------------- ================ Comment at: clang/lib/AST/FormatString.cpp:452 + } + // "partially matched" because of promotions? + if (!Ptr) { ---------------- ================ Comment at: clang/lib/AST/FormatString.cpp:401 + if (const auto *BT = argTy->getAs<BuiltinType>()) { + if (!Ptr) { + switch (BT->getKind()) { ---------------- nickdesaulniers wrote: > inclyc wrote: > > nickdesaulniers wrote: > > > aaron.ballman wrote: > > > > It's a bit strange that we have two switches over the same > > > > `BT->getKind()` and the only difference is `!Ptr`; would it be easier > > > > to read if we combined the two switches into one and had logic in the > > > > individual cases for `Ptr` vs not `Ptr`? > > > I almost made the same recommendation myself. For the below switch pair, > > > and the pair above. > > > It's a bit strange that we have two switches over the same > > > `BT->getKind()` and the only difference is `!Ptr`; would it be easier to > > > read if we combined the two switches into one and had logic in the > > > individual cases for `Ptr` vs not `Ptr`? > > > > These two switch pairs have different functions. The lower one is only > > responsible for checking whether there is a signed or unsigned integer, and > > the upper one is checking whether there is a promotion (or type confusing). > > Will they be more difficult to understand if they are written together? > Perhaps. I think the comments you added to all switches are helpful! Yeah, let's leave the structure be for now, we can always clarify it further in an NFC follow-up if it turns out to be a reasonable suggestion. ================ Comment at: clang/lib/Sema/SemaChecking.cpp:10123-10125 + // Consider character literal is a 'char' in C + // printf("%hd", 'a'); is more likey a type confusion situation + // We will suggest our users to use %hhd by discarding MatchPromotion ---------------- ================ Comment at: clang/lib/Sema/SemaChecking.cpp:10131 + if (Match == ArgType::MatchPromotion) { + if (!S.getLangOpts().ObjC && + ImplicitMatch != ArgType::NoMatchPromotionTypeConfusion && ---------------- We should probably have a comment here about why ObjC is special.. ================ Comment at: clang/www/c_status.html:822 <td><a href="https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2562.pdf";>N2562</a></td> - <td class="partial" align="center"> - <details><summary>Partial</summary> - Clang supports diagnostics checking format specifier validity, but - does not yet account for all of the changes in this paper, especially - regarding length modifiers like <code>h</code> and <code>hh</code>. - </details> - </td> + <td class="full" align="center">Clang 16</td> </tr> ---------------- 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
