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