[Sema] Use proper integral cast for an enumerate with a fixed bool type

[Mordante]

Summary

When casting an enumerate with a fixed bool type the casting should use an IntegralToBoolean instead of an IntegralCast.

Fixes PR47055: Incorrect codegen for enum with bool underlying type

[riccibruno]

clang/test/AST/ast-dump-enum-bool.cpp

1 ↗

(On Diff #284216)

Why a (pretty unreadable) json test? There is also make-ast-dump-check.sh to auto-generate an ast dump test.

[aaron.ballman]

clang/test/AST/ast-dump-enum-bool.cpp

1 ↗

(On Diff #284216)

I'd also like to see some CodeGen tests for various fixed underlying types that ensures the conversion to the underlying type happens before the conversion to the enumeration.

Also, at least one test showing this behaves properly with a C-style cast instead of a named cast.

[rsmith]

clang/test/AST/ast-dump-enum-bool.cpp

1 ↗

(On Diff #284216)

In addition to CodeGen tests, we should also test that constant evaluation does the right thing.

[Mordante]

clang/test/AST/ast-dump-enum-bool.cpp

1 ↗

(On Diff #284216)

@riccibruno Thanks for the hint! I wasn't aware of this tool. I've modified the AST test.

@aaron.ballman I've added a C-style AST test and CodeGen tests. However I'm not sure what you expect to see in the CodeGen tests. As I expected the CodeGen doesn't know about the enum, but it just uses the underlying type of the enum. Please let me know if you want to see additional tests.

[Mordante]

Addresses the review comments.

[rsmith]

clang/test/AST/ast-dump-enum-bool.cpp

1 ↗

(On Diff #284478)

I don't think we really need a dedicated AST test for this. Such tests create a maintenance burden, and they don't really capture what we care about here: that all non-zero values are correctly converted to the true value of the enumeration type.

clang/test/CodeGen/enum-bool.cpp

8–15

Some general guidance for writing IR testcases:

• Don't test things that aren't relevant to the test case; doing so will make the test brittle as IR generation changes. In particular, don't test the function attribute comments, the #0 introducing function metadata, instruction names, alignments.
• Use CHECK-LABEL for each function definition to improve matching semantics and diagnostics on mismatches. (The CHECK-LABELs are checked first, then the input is sliced up into pieces between them, and those pieces are checked independently.)
• Don't use CHECK-NEXT unless it's relevant to your test that no other instructions appear in between.

clang/test/SemaCXX/cxx2a-consteval.cpp

597–609 ↗

(On Diff #284478)

This DR test should go in test/CXX/drs/dr23xx.cpp, along with a suitable "magic comment" // dr2338: 12 to indicate this DR is implemented in Clang 12 onwards. (We have tooling that generates the clang.llvm.org/cxx_dr_status.html page from those magic comments.)

I don't think this has anything to do with consteval; a more-reduced test should work just as well (eg, static_assert((int)(E)2 == 1, "");) and would allow us to test this in C++11 onwards, not only in C++20.

[Mordante]

clang/test/AST/ast-dump-enum-bool.cpp

1 ↗

(On Diff #284478)

I'll remove the test.

clang/test/CodeGen/enum-bool.cpp

8–15

Thanks for the information, I'll reduce the test case.

clang/test/SemaCXX/cxx2a-consteval.cpp

597–609 ↗

(On Diff #284478)

I thought the magic comment could be done in any test file. It'll move the test and add the comment.

[Mordante]

Addresses review comments.

[aaron.ballman]

LGTM, thank you for the fix!

clang/lib/Sema/SemaCast.cpp

static TryCastResult TryStaticCast(Sema &Self, ExprResult &SrcExpr,
   // floating-integral conversions. Again, only floating->enum is relevant.
   if (DestType->isEnumeralType()) {
     if (Self.RequireCompleteType(OpRange.getBegin(), DestType,
                                  diag::err_bad_cast_incomplete)) {
       SrcExpr = ExprError();
       return TC_Failed;
     }
     if (SrcType->isIntegralOrEnumerationType()) {
-      Kind = CK_IntegralCast;
+      // [expr.static.cast]p10 If the enumeration type has a fixed underlying
+      // type, the value is first converted to that type by integral conversion
+      const EnumType *Enum = DestType->getAs<EnumType>();
+      Kind = Enum->getDecl()->isFixed() &&
+                     Enum->getDecl()->getIntegerType()->isBooleanType()
+                 ? CK_IntegralToBoolean
+                 : CK_IntegralCast;
       return TC_Success;
     } else if (SrcType->isRealFloatingType())   {
       Kind = CK_FloatingToIntegral;
       return TC_Success;
     }
   }
 
   // Reverse pointer upcast. C++ 4.10p3 specifies pointer upcast.

clang/test/CXX/drs/dr23xx.cpp

 // RUN: %clang_cc1 -std=c++98 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors 2>&1
 // RUN: %clang_cc1 -std=c++11 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors 2>&1 | FileCheck %s
 // RUN: %clang_cc1 -std=c++14 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors 2>&1 | FileCheck %s
 // RUN: %clang_cc1 -std=c++17 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors 2>&1 | FileCheck %s
 // RUN: %clang_cc1 -std=c++2a %s -verify -fexceptions -fcxx-exceptions -pedantic-errors 2>&1 | FileCheck %s
 
+#if __cplusplus >= 201103L
+namespace dr2338 { // dr2338: 12
+namespace B {
+enum E : bool { Zero, One };
+static_assert((int)(E)2 == 1, "");
+} // namespace B
+namespace D {
+enum class E : bool { Zero, One };
+static_assert((int)(E)2 == 1, "");
+} // namespace D
+} // namespace dr2338
+#endif
+
 namespace dr2346 { // dr2346: 11
   void test() {
     const int i2 = 0;
     extern void h2b(int x = i2 + 0); // ok, not odr-use
   }
 }
 
 namespace dr2352 { // dr2352: 10

clang/test/CodeGen/enum-bool.cpp

+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -S -emit-llvm -o - %s | FileCheck %s
+
+namespace dr2338 {
+namespace A {
+enum E { Zero, One };
+E a(int x) { return static_cast<E>(x); }
+// CHECK-LABEL: define i32 @_ZN6dr23381A1aEi
+// CHECK: ret i32 %0
+
+E b(int x) { return (E)x; }
+// CHECK-LABEL: define i32 @_ZN6dr23381A1bEi
+// CHECK: ret i32 %0
+
+} // namespace A
+namespace B {
+enum E : bool { Zero, One };
+E a(int x) { return static_cast<E>(x); }
+// CHECK-LABEL: define zeroext i1 @_ZN6dr23381B1aEi
+// CHECK: ret i1 %tobool
+
+E b(int x) { return (E)x; }
+// CHECK-LABEL: define zeroext i1 @_ZN6dr23381B1bEi
+// CHECK: ret i1 %tobool
+
+} // namespace B
+namespace C {
+enum class E { Zero, One };
+E a(int x) { return static_cast<E>(x); }
+// CHECK-LABEL: define i32 @_ZN6dr23381C1aEi
+// CHECK: ret i32 %0
+
+E b(int x) { return (E)x; }
+// CHECK-LABEL: define i32 @_ZN6dr23381C1bEi
+// CHECK: ret i32 %0
+
+} // namespace C
+namespace D {
+enum class E : bool { Zero, One };
+E a(int x) { return static_cast<E>(x); }
+// CHECK-LABEL: define zeroext i1 @_ZN6dr23381D1aEi
+// CHECK: ret i1 %tobool
+
+E b(int x) { return (E)x; }
+
+// CHECK-LABEL: define zeroext i1 @_ZN6dr23381D1bEi
+// CHECK: ret i1 %tobool
+
+} // namespace D
+} // namespace dr2338