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

[Mark de Wever via Phabricator via cfe-commits]

Mordante marked 2 inline comments as done.
Mordante added inline comments.


================
Comment at: clang/test/AST/ast-dump-enum-bool.cpp:1
+// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-unknown -fsyntax-only -ast-dump %s | FileCheck %s
+
----------------
rsmith wrote:
> 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.
I'll remove the test.


================
Comment at: clang/test/CodeGen/enum-bool.cpp:7-14
+// CHECK:      ; Function Attrs: noinline nounwind optnone
+// CHECK-NEXT: define i32 @_ZN6dr23381A1aEi(i32 %x) #0 {
+// CHECK-NEXT: entry:
+// CHECK-NEXT:   %x.addr = alloca i32, align 4
+// CHECK-NEXT:   store i32 %x, i32* %x.addr, align 4
+// CHECK-NEXT:   %0 = load i32, i32* %x.addr, align 4
+// CHECK-NEXT:   ret i32 %0
----------------
rsmith wrote:
> 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-LABEL`s 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.
Thanks for the information, I'll reduce the test case.


================
Comment at: clang/test/SemaCXX/cxx2a-consteval.cpp:597-609
+
+namespace dr2338 {
+namespace B {
+enum E : bool { Zero, One };
+consteval E c(int x) { return (E)x; }
+static_assert(static_cast<int>(c(2)) == 1);
+} // namespace B
----------------
rsmith wrote:
> 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.
I thought the magic comment could be done in any test file. It'll move the test and add the comment.


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

https://reviews.llvm.org/D85612