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

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:15775
+      if (Context.typesAreCompatible(PromoteType, UnderlyingType,
+                                     /*CompareUnqualified*/ true))
         PromoteType = QualType();
----------------
If we're not going to take advantage of the C notion of compatibility, might as well just check hasSameType().


================
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() &&
----------------
aaron.ballman wrote:
> 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?
That makes more sense.

Not sure this particular issue can show up in C; there's a check for C++ in Sema::ActOnEnumBody.  But no harm at least.


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

https://reviews.llvm.org/D103611