[PATCH] D103611: Correct the behavior of va_arg checking in C++

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:15783
+      // test for typesAreCompatible() will already properly consider those to
+      // be compatible types.
+      if (Context.getLangOpts().CPlusPlus && !PromoteType.isNull() &&
----------------
efriedma wrote:
> This explanation doesn't seem right.  Signed and unsigned types are never considered "compatible".
> 
> If I'm understanding correctly, the case this code addresses is promotion according to `[conv.prom]`p3: "A prvalue of an unscoped enumeration type whose underlying type is not fixed [...]".  Somehow, the enum ends up with an unsigned underlying type, but we promote to int?  And this doesn't happen in C somehow?
> This explanation doesn't seem right. Signed and unsigned types are never considered "compatible".

Good point, I think that comment is wrong.

> If I'm understanding correctly, the case this code addresses is promotion according to [conv.prom]p3: "A prvalue of an unscoped enumeration type whose underlying type is not fixed [...]". Somehow, the enum ends up with an unsigned underlying type, but we promote to int? And this doesn't happen in C somehow?

That's correct. What I am seeing is:
```
enum Unscoped { One = 0x7FFFFFFF };
```
C++:
`PromoteType` = Builtin (Int)
`UnderlyingType` = Builtin (UInt)

C:
`PromoteType` = Builtin (UInt)
`UnderlyingType` = Builtin (UInt)


`enum Unscoped { One = 0xFFFFFFFF };`

C++:
`PromoteType` = Builtin (UInt)
`UnderlyingType` = Builtin (UInt)

C:
`PromoteType` = Builtin (UInt)
`UnderlyingType` = Builtin (UInt)

At least on i386-pc-unknown.

So I think this code is almost correct for that test, but is over-constrained by only doing this in C++. WDYT?


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

https://reviews.llvm.org/D103611