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

[Aaron Ballman via Phabricator via cfe-commits]

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