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

[YingChi Long via Phabricator via cfe-commits]

inclyc added inline comments.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:10127-10129
+    if (ImplicitMatch != ArgType::NoMatchPromotionTypeConfusion &&
+        ImplicitMatch != ArgType::NoMatchTypeConfusion &&
+        !IsCharacterLiteralInt)
----------------
nickdesaulniers wrote:
> There's something about this conditional...I can't quite put my finger on.
> 
> Isn't `ImplicitMatch` only updated in the `const ImplicitCastExpr *ICE = dyn_cast<ImplicitCastExpr>(E)` branch above, where `IsCharacterLiteralInt` cannot be?
> 
> Simultaneously, isn't `IsCharacterLiteralInt` only updated in the `const CharacterLiteral *CL = dyn_cast<CharacterLiteral>(E)` branch above, where `ImplicitMatch` cannot be?
> 
> I wonder if we should instead hoist:
> ```
> if (Match == ArgType::MatchPromotion) {
>   if (ImplicitMatch != ArgType::NoMatchPromotionTypeConfusion &&
>       ImplicitMatch != ArgType::NoMatchTypeConfusion)
>     return true;
>   Match = ArgType::NoMatch;
> }
> ```
> into the `const ImplicitCastExpr *ICE = dyn_cast<ImplicitCastExpr>(E)` branch after `ImplicitMatch` is updated.
> 
> Then hoist
> ```
> if (Match == ArgType::MatchPromotion)
>   return true;
> ```
> into the `const CharacterLiteral *CL = dyn_cast<CharacterLiteral>(E)` branch after `IsCharacterLiteralInt` is updated? Then we could delete the `IsCharacterLiteralInt` variable perhaps?
> There's something about this conditional...I can't quite put my finger on.
> 
> Isn't `ImplicitMatch` only updated in the `const ImplicitCastExpr *ICE = dyn_cast<ImplicitCastExpr>(E)` branch above, where `IsCharacterLiteralInt` cannot be?
> 
> Simultaneously, isn't `IsCharacterLiteralInt` only updated in the `const CharacterLiteral *CL = dyn_cast<CharacterLiteral>(E)` branch above, where `ImplicitMatch` cannot be?
> 
> I wonder if we should instead hoist:
> ```
> if (Match == ArgType::MatchPromotion) {
>   if (ImplicitMatch != ArgType::NoMatchPromotionTypeConfusion &&
>       ImplicitMatch != ArgType::NoMatchTypeConfusion)
>     return true;
>   Match = ArgType::NoMatch;
> }
> ```
> into the `const ImplicitCastExpr *ICE = dyn_cast<ImplicitCastExpr>(E)` branch after `ImplicitMatch` is updated.


The control flow may not entering any of these branches at all, and if we have `Match == MatchPromotion` we depend on the origin value of `ImplicitMatch` to determine whether or not to discard the `MatchPromotion` property. For example, 

```
printf("%hd", 0);
```

We have `MatchPromotion` because the argument is an `int` and the format string specifies `signed short`, and we will not entering `ImplicitCastExpr` branch because the argument is an `int` already (so we don't need an "implicit cast")

> 
> Then hoist
> ```
> if (Match == ArgType::MatchPromotion)
>   return true;
> ```
> into the `const CharacterLiteral *CL = dyn_cast<CharacterLiteral>(E)` branch after `IsCharacterLiteralInt` is updated? Then we could delete the `IsCharacterLiteralInt` variable perhaps?

Thank you for this! I've hoisted this into `CharacterLiteralExpr` branch. And now `IsCharacterLiteralInt` is unnecessary.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132568/new/

https://reviews.llvm.org/D132568