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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
