[clang] [Clang][Sema] Fix type of enumerators in incomplete enumerations (PR #84068)
https://github.com/AaronBallman closed https://github.com/llvm/llvm-project/pull/84068 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] Fix type of enumerators in incomplete enumerations (PR #84068)
@@ -1,12 +1,19 @@ // RUN: %clang_cc1 -x c -fsyntax-only -verify -Wenum-compare -Wno-unused-comparison %s // RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wenum-compare -Wno-unused-comparison %s +// In C enumerators (i.e enumeration constants) have type int (until C23). In order to support diagnostics such as Kupa-Martin wrote: done https://github.com/llvm/llvm-project/pull/84068 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] Fix type of enumerators in incomplete enumerations (PR #84068)
@@ -1,12 +1,19 @@
// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wenum-compare
-Wno-unused-comparison %s
// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wenum-compare
-Wno-unused-comparison %s
+// In C enumerators (i.e enumeration constants) have type int (until C23). In
order to support diagnostics such as
+// -Wenum-compare we pretend they have the type of their enumeration.
+
typedef enum EnumA {
A
} EnumA;
enum EnumB {
- B
+ B,
+ B1 = 1,
+ // In C++ this comparison doesnt warn as enumerators dont have the type of
their enumeration before the closing
Kupa-Martin wrote:
done
https://github.com/llvm/llvm-project/pull/84068
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] Fix type of enumerators in incomplete enumerations (PR #84068)
https://github.com/Kupa-Martin updated
https://github.com/llvm/llvm-project/pull/84068
>From cc6a1329761a8fa5ae0b9f51ef8b7f839d5edff2 Mon Sep 17 00:00:00 2001
From: 44-2-Kupa-Martin
Date: Tue, 5 Mar 2024 17:21:02 -0300
Subject: [PATCH] [Clang][Sema] Fix type of enumerators in incomplete
enumerations Enumerators dont have the type of their enumeration before the
closing brace. In these cases Expr::getEnumCoercedType() incorrectly returned
the enumeration type.
Introduced in PR #81418
Fixes #84712
---
clang/lib/AST/Expr.cpp | 13 -
clang/test/Sema/enum-constant-type.cpp | 12
clang/test/Sema/warn-compare-enum-types-mismatch.c | 11 ++-
3 files changed, 30 insertions(+), 6 deletions(-)
create mode 100644 clang/test/Sema/enum-constant-type.cpp
diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
index b4de2155adcebd..f5ad402e3bd73e 100644
--- a/clang/lib/AST/Expr.cpp
+++ b/clang/lib/AST/Expr.cpp
@@ -264,11 +264,14 @@ namespace {
}
QualType Expr::getEnumCoercedType(const ASTContext ) const {
- if (isa(this->getType()))
-return this->getType();
- else if (const auto *ECD = this->getEnumConstantDecl())
-return Ctx.getTypeDeclType(cast(ECD->getDeclContext()));
- return this->getType();
+ if (isa(getType()))
+return getType();
+ if (const auto *ECD = getEnumConstantDecl()) {
+const auto *ED = cast(ECD->getDeclContext());
+if (ED->isCompleteDefinition())
+ return Ctx.getTypeDeclType(ED);
+ }
+ return getType();
}
SourceLocation Expr::getExprLoc() const {
diff --git a/clang/test/Sema/enum-constant-type.cpp
b/clang/test/Sema/enum-constant-type.cpp
new file mode 100644
index 00..5db3a859a39599
--- /dev/null
+++ b/clang/test/Sema/enum-constant-type.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify %s -Wenum-compare
+// expected-no-diagnostics
+
+enum E1 {
+ E11 = 0
+};
+
+enum E2 {
+ E21 = 0,
+ E22 = E11,
+ E23 = E21 + E22
+};
diff --git a/clang/test/Sema/warn-compare-enum-types-mismatch.c
b/clang/test/Sema/warn-compare-enum-types-mismatch.c
index 2b72aae16b977a..47dd592488e6dc 100644
--- a/clang/test/Sema/warn-compare-enum-types-mismatch.c
+++ b/clang/test/Sema/warn-compare-enum-types-mismatch.c
@@ -1,12 +1,21 @@
// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wenum-compare
-Wno-unused-comparison %s
// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wenum-compare
-Wno-unused-comparison %s
+// In C enumerators (i.e enumeration constants) have type int (until C23). In
+// order to support diagnostics such as -Wenum-compare we pretend they have the
+// type of their enumeration.
+
typedef enum EnumA {
A
} EnumA;
enum EnumB {
- B
+ B,
+ B1 = 1,
+ // In C++ this comparison doesnt warn as enumerators dont have the type of
+ // their enumeration before the closing brace. We mantain the same behavior
+ // in C.
+ B2 = A == B1
};
enum {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] Fix type of enumerators in incomplete enumerations (PR #84068)
https://github.com/AaronBallman approved this pull request. Aside from some formatting issues, this LGTM. Once you push up a fix for the formatting, I'll land. Thank you! https://github.com/llvm/llvm-project/pull/84068 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] Fix type of enumerators in incomplete enumerations (PR #84068)
@@ -1,12 +1,19 @@ // RUN: %clang_cc1 -x c -fsyntax-only -verify -Wenum-compare -Wno-unused-comparison %s // RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wenum-compare -Wno-unused-comparison %s +// In C enumerators (i.e enumeration constants) have type int (until C23). In order to support diagnostics such as AaronBallman wrote: There's trailing whitespace here and it goes over the 80-col limit; can you reformat? https://github.com/llvm/llvm-project/pull/84068 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] Fix type of enumerators in incomplete enumerations (PR #84068)
@@ -1,12 +1,19 @@
// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wenum-compare
-Wno-unused-comparison %s
// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wenum-compare
-Wno-unused-comparison %s
+// In C enumerators (i.e enumeration constants) have type int (until C23). In
order to support diagnostics such as
+// -Wenum-compare we pretend they have the type of their enumeration.
+
typedef enum EnumA {
A
} EnumA;
enum EnumB {
- B
+ B,
+ B1 = 1,
+ // In C++ this comparison doesnt warn as enumerators dont have the type of
their enumeration before the closing
AaronBallman wrote:
Same issue here as above with trailing whitespace and 80 col.
https://github.com/llvm/llvm-project/pull/84068
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] Fix type of enumerators in incomplete enumerations (PR #84068)
https://github.com/AaronBallman edited https://github.com/llvm/llvm-project/pull/84068 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] Fix type of enumerators in incomplete enumerations (PR #84068)
Kupa-Martin wrote: > > @shafik We dont have a dedicated cpp test for this. I can add one if you > > want, but clang/test/Sema/warn-compare-enum-types-mismatch.c runs clang > > both on C and C++ mode, so I didnt think it necessary. > > I think we just a bug that demonstrates this issue: #84712 > > So if this fixes that as well then we should add tests for those cases too. I've included a separate test for C++. If there are no more changes, could someone please merge this for me? I dont have commit access. https://github.com/llvm/llvm-project/pull/84068 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] Fix type of enumerators in incomplete enumerations (PR #84068)
https://github.com/Kupa-Martin edited https://github.com/llvm/llvm-project/pull/84068 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] Fix type of enumerators in incomplete enumerations (PR #84068)
https://github.com/Kupa-Martin updated
https://github.com/llvm/llvm-project/pull/84068
>From 57538a2e2b9446cf692d11386b3560225eb86ce4 Mon Sep 17 00:00:00 2001
From: 44-2-Kupa-Martin
Date: Tue, 5 Mar 2024 17:21:02 -0300
Subject: [PATCH] [Clang][Sema] Fix type of enumerators in incomplete
enumerations Enumerators dont have the type of their enumeration before the
closing brace. In these cases Expr::getEnumCoercedType() incorrectly returned
the enumeration type.
Introduced in PR #81418
Fixes #84712
---
clang/lib/AST/Expr.cpp | 13 -
clang/test/Sema/enum-constant-type.cpp | 12
clang/test/Sema/warn-compare-enum-types-mismatch.c | 9 -
3 files changed, 28 insertions(+), 6 deletions(-)
create mode 100644 clang/test/Sema/enum-constant-type.cpp
diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
index b4de2155adcebd..f5ad402e3bd73e 100644
--- a/clang/lib/AST/Expr.cpp
+++ b/clang/lib/AST/Expr.cpp
@@ -264,11 +264,14 @@ namespace {
}
QualType Expr::getEnumCoercedType(const ASTContext ) const {
- if (isa(this->getType()))
-return this->getType();
- else if (const auto *ECD = this->getEnumConstantDecl())
-return Ctx.getTypeDeclType(cast(ECD->getDeclContext()));
- return this->getType();
+ if (isa(getType()))
+return getType();
+ if (const auto *ECD = getEnumConstantDecl()) {
+const auto *ED = cast(ECD->getDeclContext());
+if (ED->isCompleteDefinition())
+ return Ctx.getTypeDeclType(ED);
+ }
+ return getType();
}
SourceLocation Expr::getExprLoc() const {
diff --git a/clang/test/Sema/enum-constant-type.cpp
b/clang/test/Sema/enum-constant-type.cpp
new file mode 100644
index 00..5db3a859a39599
--- /dev/null
+++ b/clang/test/Sema/enum-constant-type.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify %s -Wenum-compare
+// expected-no-diagnostics
+
+enum E1 {
+ E11 = 0
+};
+
+enum E2 {
+ E21 = 0,
+ E22 = E11,
+ E23 = E21 + E22
+};
diff --git a/clang/test/Sema/warn-compare-enum-types-mismatch.c
b/clang/test/Sema/warn-compare-enum-types-mismatch.c
index 2b72aae16b977a..85d218e3213b2d 100644
--- a/clang/test/Sema/warn-compare-enum-types-mismatch.c
+++ b/clang/test/Sema/warn-compare-enum-types-mismatch.c
@@ -1,12 +1,19 @@
// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wenum-compare
-Wno-unused-comparison %s
// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wenum-compare
-Wno-unused-comparison %s
+// In C enumerators (i.e enumeration constants) have type int (until C23). In
order to support diagnostics such as
+// -Wenum-compare we pretend they have the type of their enumeration.
+
typedef enum EnumA {
A
} EnumA;
enum EnumB {
- B
+ B,
+ B1 = 1,
+ // In C++ this comparison doesnt warn as enumerators dont have the type of
their enumeration before the closing
+ // brace. We mantain the same behavior in C.
+ B2 = A == B1
};
enum {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] Fix type of enumerators in incomplete enumerations (PR #84068)
Sirraide wrote:
Regarding #84712: From what I can tell, the behaviour of cases such as
```c++
enum e1 { a };
enum e2 { b = a };
```
has changed between C++11 and C++14 (at least the wording is different starting
with C++14; there may be some other section that states the same for C++11, but
I couldn’t find one); specifically, C++11’s
[[dcl.enum]p5](https://eel.is/c++draft/enum#dcl.enum-5) states that:
> If the underlying type is not fixed, the type of each enumerator is the type
of its initializing value:
> - If an initializer is specified for an enumerator, the initializing value
> **has the same type as the expression**
and the constant-expression shall be an integral constant expression (5.19)
Whereas in C++14, that same section reads:
> If the underlying type is not fixed, the type of each enumerator prior
to the closing brace is determined as follows:
> - If an initializer is specified for an enumerator, the constant-expression
> shall be an integral constant
expression (5.20). **If the expression has unscoped enumeration type, the
enumerator has the underlying
type of that enumeration type, otherwise it has the same type as the
expression**.
That is, not only is the type of `b` above not `e2` before the closing brace,
but it would seem that its type is supposed to be `e1` before C++14, and
whatever the underlying type of `e1` is starting with C++14.
https://github.com/llvm/llvm-project/pull/84068
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] Fix type of enumerators in incomplete enumerations (PR #84068)
shafik wrote: > @shafik We dont have a dedicated cpp test for this. I can add one if you > want, but clang/test/Sema/warn-compare-enum-types-mismatch.c runs clang both > on C and C++ mode, so I didnt think it necessary. I think we just a bug that demonstrates this issue: https://github.com/llvm/llvm-project/issues/84712 So if this fixes that as well then we should add tests for those cases too. https://github.com/llvm/llvm-project/pull/84068 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] Fix type of enumerators in incomplete enumerations (PR #84068)
Kupa-Martin wrote: @shafik We dont have a dedicated cpp test for this. I can add one if you want, but clang/test/Sema/warn-compare-enum-types-mismatch.c runs clang both on C and C++ mode, so I didnt think it necessary. https://github.com/llvm/llvm-project/pull/84068 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] Fix type of enumerators in incomplete enumerations (PR #84068)
@@ -6,7 +6,9 @@ typedef enum EnumA {
} EnumA;
enum EnumB {
- B
+ B,
shafik wrote:
I think what I was asking, was do we have an equivalent C++ test that verifies
in a `.cpp` file that we also do not obtain a diagnostic for this.
https://github.com/llvm/llvm-project/pull/84068
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] Fix type of enumerators in incomplete enumerations (PR #84068)
shafik wrote:
> So I believe:
>
> ```
> typedef enum EnumA {
> A
> } EnumA;
>
> enum EnumB {
> B,
> B1 = 1,
> B2 = A == B1
> };
> ```
>
> is not an enum compare warning in C++ because `B1` doesn't have an
> enumeration type due to the enumeration not being fully-defined, and is not
> an enum compare warning in C because `A` has type `int` (due to p15) and `B1`
> has type `int` (due to p12).
Right, so do we have a test for C++ that verifies that.
https://github.com/llvm/llvm-project/pull/84068
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] Fix type of enumerators in incomplete enumerations (PR #84068)
@@ -6,7 +6,9 @@ typedef enum EnumA {
} EnumA;
enum EnumB {
- B
+ B,
Kupa-Martin wrote:
done
https://github.com/llvm/llvm-project/pull/84068
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] Fix type of enumerators in incomplete enumerations (PR #84068)
@@ -264,11 +264,14 @@ namespace {
}
QualType Expr::getEnumCoercedType(const ASTContext ) const {
- if (isa(this->getType()))
-return this->getType();
- else if (const auto *ECD = this->getEnumConstantDecl())
-return Ctx.getTypeDeclType(cast(ECD->getDeclContext()));
- return this->getType();
+ if (isa(getType())) {
+return getType();
+ } else if (const auto *ECD = getEnumConstantDecl()) {
+const auto *ED = cast(ECD->getDeclContext());
+if (ED->isCompleteDefinition())
+ return Ctx.getTypeDeclType(ED);
+ }
Kupa-Martin wrote:
done
https://github.com/llvm/llvm-project/pull/84068
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] Fix type of enumerators in incomplete enumerations (PR #84068)
https://github.com/Kupa-Martin updated
https://github.com/llvm/llvm-project/pull/84068
>From 4bbd5cf7eb1eeeaba8eed2ac4aba76cdbbcc671f Mon Sep 17 00:00:00 2001
From: 44-2-Kupa-Martin
Date: Tue, 5 Mar 2024 17:21:02 -0300
Subject: [PATCH] [Clang][Sema] Fix type of enumerators in incomplete
enumerations Enumerators dont have the type of their enumeration before the
closing brace. In these cases Expr::getEnumCoercedType() incorrectly returned
the enumeration type.
Introduced in PR #81418
---
clang/lib/AST/Expr.cpp | 13 -
clang/test/Sema/warn-compare-enum-types-mismatch.c | 9 -
2 files changed, 16 insertions(+), 6 deletions(-)
diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
index b4de2155adcebd..f5ad402e3bd73e 100644
--- a/clang/lib/AST/Expr.cpp
+++ b/clang/lib/AST/Expr.cpp
@@ -264,11 +264,14 @@ namespace {
}
QualType Expr::getEnumCoercedType(const ASTContext ) const {
- if (isa(this->getType()))
-return this->getType();
- else if (const auto *ECD = this->getEnumConstantDecl())
-return Ctx.getTypeDeclType(cast(ECD->getDeclContext()));
- return this->getType();
+ if (isa(getType()))
+return getType();
+ if (const auto *ECD = getEnumConstantDecl()) {
+const auto *ED = cast(ECD->getDeclContext());
+if (ED->isCompleteDefinition())
+ return Ctx.getTypeDeclType(ED);
+ }
+ return getType();
}
SourceLocation Expr::getExprLoc() const {
diff --git a/clang/test/Sema/warn-compare-enum-types-mismatch.c
b/clang/test/Sema/warn-compare-enum-types-mismatch.c
index 2b72aae16b977a..85d218e3213b2d 100644
--- a/clang/test/Sema/warn-compare-enum-types-mismatch.c
+++ b/clang/test/Sema/warn-compare-enum-types-mismatch.c
@@ -1,12 +1,19 @@
// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wenum-compare
-Wno-unused-comparison %s
// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wenum-compare
-Wno-unused-comparison %s
+// In C enumerators (i.e enumeration constants) have type int (until C23). In
order to support diagnostics such as
+// -Wenum-compare we pretend they have the type of their enumeration.
+
typedef enum EnumA {
A
} EnumA;
enum EnumB {
- B
+ B,
+ B1 = 1,
+ // In C++ this comparison doesnt warn as enumerators dont have the type of
their enumeration before the closing
+ // brace. We mantain the same behavior in C.
+ B2 = A == B1
};
enum {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] Fix type of enumerators in incomplete enumerations (PR #84068)
Kupa-Martin wrote: > not an enum compare warning in C++ because `B1` doesn't have an enumeration > type due to the enumeration not being fully-defined, and is not an enum > compare warning in C because `A` has type `int` (due to p15) and `B1` has > type `int` (due to p12). Yes, you are correct. > I think we play a bit fast-and-loose with the way we treat enum constants in > C in Clang, but that's outside of the scope of this patch. There are enough differences between the C23 and C++ standards to produce different diagnostics. I've made a godbolt [snippet](https://godbolt.org/z/741W1356r) that tries to keep track of the type of different enumerators in C23 and C++; we warn when we shouldnt and we dont warn when we should. In any case, this patch didnt intend to cover C23. https://github.com/llvm/llvm-project/pull/84068 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] Fix type of enumerators in incomplete enumerations (PR #84068)
@@ -6,7 +6,9 @@ typedef enum EnumA {
} EnumA;
enum EnumB {
- B
+ B,
AaronBallman wrote:
It might be good to capture some of what I wrote in the review summary as a
comment here so it's clear why this is correct in both C and C++.
https://github.com/llvm/llvm-project/pull/84068
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] Fix type of enumerators in incomplete enumerations (PR #84068)
@@ -264,11 +264,14 @@ namespace {
}
QualType Expr::getEnumCoercedType(const ASTContext ) const {
- if (isa(this->getType()))
-return this->getType();
- else if (const auto *ECD = this->getEnumConstantDecl())
-return Ctx.getTypeDeclType(cast(ECD->getDeclContext()));
- return this->getType();
+ if (isa(getType())) {
+return getType();
+ } else if (const auto *ECD = getEnumConstantDecl()) {
+const auto *ED = cast(ECD->getDeclContext());
+if (ED->isCompleteDefinition())
+ return Ctx.getTypeDeclType(ED);
+ }
AaronBallman wrote:
```suggestion
if (isa(getType()))
return getType();
if (const auto *ECD = getEnumConstantDecl()) {
const auto *ED = cast(ECD->getDeclContext());
if (ED->isCompleteDefinition())
return Ctx.getTypeDeclType(ED);
}
```
No `else` after `return` via our coding standard, NFC
https://github.com/llvm/llvm-project/pull/84068
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] Fix type of enumerators in incomplete enumerations (PR #84068)
https://github.com/AaronBallman edited https://github.com/llvm/llvm-project/pull/84068 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] Fix type of enumerators in incomplete enumerations (PR #84068)
https://github.com/AaronBallman approved this pull request.
Hmm, the rules are different between C and C++, but I think we may be okay. In
C++:
https://eel.is/c++draft/enum#dcl.enum-5.sentence-6
Following the closing brace of an enum-specifier, each enumerator has the type
of its enumeration.
In C23 6.7.3.3p12:
During the processing of each enumeration constant in the enumerator list, the
type of the enumeration constant shall be:
p15:
The enumeration member type for an enumerated type without fixed underlying
type upon completion is:
— int if all the values of the enumeration are representable as an int; or,
— the enumerated type.140)
Footnote 140) The integer type selected during processing of the enumerator
list (before completion) of the enumeration may not be the same as the
compatible implementation-defined integer type selected for the completed
enumeration.
So I believe:
```
typedef enum EnumA {
A
} EnumA;
enum EnumB {
B,
B1 = 1,
B2 = A == B1
};
```
is not an enum compare warning in C++ because `B1` doesn't have an enumeration
type due to the enumeration not being fully-defined, and is not an enum compare
warning in C because `A` has type `int` (due to p15) and `B1` has type `int`
(due to p12).
I think we play a bit fast-and-loose with the way we treat enum constants in C
in Clang, but that's outside of the scope of this patch. So LGTM aside from
some tiny nits!
https://github.com/llvm/llvm-project/pull/84068
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] Fix type of enumerators in incomplete enumerations (PR #84068)
Kupa-Martin wrote: > Should we also have a C++ test for this fix? clang/test/Sema/warn-compare-enum-types-mismatch.c should cover both C and C++. Or do you mean some other kind of test? https://github.com/llvm/llvm-project/pull/84068 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] Fix type of enumerators in incomplete enumerations (PR #84068)
https://github.com/shafik commented: Should we also have a C++ test for this fix? https://github.com/llvm/llvm-project/pull/84068 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] Fix type of enumerators in incomplete enumerations (PR #84068)
erichkeane wrote: > > > Also, this probably needs a release note. > > > > > > If you want I'll add one but this bug has been on main no longer than a > > week, so I didnt think it would be necessary. > > I see. Yeah, I don’t think we really need one if the bug was introduced and > fixed in the same version. Thats correct, a release note is only necessary in cases where the bug existed in a release. https://github.com/llvm/llvm-project/pull/84068 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] Fix type of enumerators in incomplete enumerations (PR #84068)
Sirraide wrote: > > Also, this probably needs a release note. > > If you want I'll add one but this bug has been on main no longer than a week, > so I didnt think it would be necessary. I see. Yeah, I don’t think we really need one if the bug was introduced and fixed in the same version. https://github.com/llvm/llvm-project/pull/84068 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] Fix type of enumerators in incomplete enumerations (PR #84068)
@@ -264,10 +264,13 @@ namespace {
}
QualType Expr::getEnumCoercedType(const ASTContext ) const {
- if (isa(this->getType()))
+ if (isa(this->getType())) {
return this->getType();
- else if (const auto *ECD = this->getEnumConstantDecl())
-return Ctx.getTypeDeclType(cast(ECD->getDeclContext()));
+ } else if (const auto *ECD = this->getEnumConstantDecl()) {
+const auto *ED = cast(ECD->getDeclContext());
+if (ED->isCompleteDefinition())
+ return Ctx.getTypeDeclType(ED);
+ }
return this->getType();
Kupa-Martin wrote:
Force of habit. I already changed it.
https://github.com/llvm/llvm-project/pull/84068
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] Fix type of enumerators in incomplete enumerations (PR #84068)
https://github.com/Kupa-Martin updated
https://github.com/llvm/llvm-project/pull/84068
>From 92c0ebc1fa419404dfa8c2497248cc35c2644c5b Mon Sep 17 00:00:00 2001
From: 44-2-Kupa-Martin
Date: Tue, 5 Mar 2024 17:21:02 -0300
Subject: [PATCH] [Clang][Sema] Fix type of enumerators in incomplete
enumerations Enumerators dont have the type of their enumeration before the
closing brace. In these cases Expr::getEnumCoercedType() incorrectly returned
the enumeration type.
Introduced in PR #81418
---
clang/lib/AST/Expr.cpp | 13 -
clang/test/Sema/warn-compare-enum-types-mismatch.c | 4 +++-
2 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
index b4de2155adcebd..fd0faa98c07178 100644
--- a/clang/lib/AST/Expr.cpp
+++ b/clang/lib/AST/Expr.cpp
@@ -264,11 +264,14 @@ namespace {
}
QualType Expr::getEnumCoercedType(const ASTContext ) const {
- if (isa(this->getType()))
-return this->getType();
- else if (const auto *ECD = this->getEnumConstantDecl())
-return Ctx.getTypeDeclType(cast(ECD->getDeclContext()));
- return this->getType();
+ if (isa(getType())) {
+return getType();
+ } else if (const auto *ECD = getEnumConstantDecl()) {
+const auto *ED = cast(ECD->getDeclContext());
+if (ED->isCompleteDefinition())
+ return Ctx.getTypeDeclType(ED);
+ }
+ return getType();
}
SourceLocation Expr::getExprLoc() const {
diff --git a/clang/test/Sema/warn-compare-enum-types-mismatch.c
b/clang/test/Sema/warn-compare-enum-types-mismatch.c
index 2b72aae16b977a..1094a6972e778d 100644
--- a/clang/test/Sema/warn-compare-enum-types-mismatch.c
+++ b/clang/test/Sema/warn-compare-enum-types-mismatch.c
@@ -6,7 +6,9 @@ typedef enum EnumA {
} EnumA;
enum EnumB {
- B
+ B,
+ B1 = 1,
+ B2 = A == B1
};
enum {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] Fix type of enumerators in incomplete enumerations (PR #84068)
Kupa-Martin wrote: > Also, this probably needs a release note. If you want I'll add one but this bug has been on main no longer than a week, so I didnt think it would be necessary. https://github.com/llvm/llvm-project/pull/84068 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] Fix type of enumerators in incomplete enumerations (PR #84068)
Sirraide wrote: Also, this probably needs a release note. https://github.com/llvm/llvm-project/pull/84068 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] Fix type of enumerators in incomplete enumerations (PR #84068)
@@ -264,10 +264,13 @@ namespace {
}
QualType Expr::getEnumCoercedType(const ASTContext ) const {
- if (isa(this->getType()))
+ if (isa(this->getType())) {
return this->getType();
- else if (const auto *ECD = this->getEnumConstantDecl())
-return Ctx.getTypeDeclType(cast(ECD->getDeclContext()));
+ } else if (const auto *ECD = this->getEnumConstantDecl()) {
+const auto *ED = cast(ECD->getDeclContext());
+if (ED->isCompleteDefinition())
+ return Ctx.getTypeDeclType(ED);
+ }
return this->getType();
Sirraide wrote:
Erm, is there a good reason why we’re doing `this->` here? I can see it was
doing that before too, but it just seems really unnecessary to me...
https://github.com/llvm/llvm-project/pull/84068
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] Fix type of enumerators in incomplete enumerations (PR #84068)
Kupa-Martin wrote: @erichkeane I cant add the previous reviewers myself, could you do it for me please? https://github.com/llvm/llvm-project/pull/84068 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] Fix type of enumerators in incomplete enumerations (PR #84068)
llvmbot wrote:
@llvm/pr-subscribers-clang
Author: None (Kupa-Martin)
Changes
Enumerators dont have the type of their enumeration before the closing brace.
In these cases Expr::getEnumCoercedType() incorrectly returned the enumeration
type.
Introduced in PR #81418
---
Full diff: https://github.com/llvm/llvm-project/pull/84068.diff
2 Files Affected:
- (modified) clang/lib/AST/Expr.cpp (+6-3)
- (modified) clang/test/Sema/warn-compare-enum-types-mismatch.c (+3-1)
``diff
diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
index b4de2155adcebd..9a28e6f750069b 100644
--- a/clang/lib/AST/Expr.cpp
+++ b/clang/lib/AST/Expr.cpp
@@ -264,10 +264,13 @@ namespace {
}
QualType Expr::getEnumCoercedType(const ASTContext ) const {
- if (isa(this->getType()))
+ if (isa(this->getType())) {
return this->getType();
- else if (const auto *ECD = this->getEnumConstantDecl())
-return Ctx.getTypeDeclType(cast(ECD->getDeclContext()));
+ } else if (const auto *ECD = this->getEnumConstantDecl()) {
+const auto *ED = cast(ECD->getDeclContext());
+if (ED->isCompleteDefinition())
+ return Ctx.getTypeDeclType(ED);
+ }
return this->getType();
}
diff --git a/clang/test/Sema/warn-compare-enum-types-mismatch.c
b/clang/test/Sema/warn-compare-enum-types-mismatch.c
index 2b72aae16b977a..1094a6972e778d 100644
--- a/clang/test/Sema/warn-compare-enum-types-mismatch.c
+++ b/clang/test/Sema/warn-compare-enum-types-mismatch.c
@@ -6,7 +6,9 @@ typedef enum EnumA {
} EnumA;
enum EnumB {
- B
+ B,
+ B1 = 1,
+ B2 = A == B1
};
enum {
``
https://github.com/llvm/llvm-project/pull/84068
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] Fix type of enumerators in incomplete enumerations (PR #84068)
https://github.com/Kupa-Martin created
https://github.com/llvm/llvm-project/pull/84068
Enumerators dont have the type of their enumeration before the closing brace.
In these cases Expr::getEnumCoercedType() incorrectly returned the enumeration
type.
Introduced in PR #81418
>From bc9f93713a07643aa3095a7d6d402d1202e07b9e Mon Sep 17 00:00:00 2001
From: 44-2-Kupa-Martin
Date: Tue, 5 Mar 2024 17:21:02 -0300
Subject: [PATCH] [Clang][Sema] Fix type of enumerators in incomplete
enumerations Enumerators dont have the type of their enumeration before the
closing brace. In these cases Expr::getEnumCoercedType() incorrectly returned
the enumeration type.
Introduced in PR #81418
---
clang/lib/AST/Expr.cpp | 9 ++---
clang/test/Sema/warn-compare-enum-types-mismatch.c | 4 +++-
2 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
index b4de2155adcebd..9a28e6f750069b 100644
--- a/clang/lib/AST/Expr.cpp
+++ b/clang/lib/AST/Expr.cpp
@@ -264,10 +264,13 @@ namespace {
}
QualType Expr::getEnumCoercedType(const ASTContext ) const {
- if (isa(this->getType()))
+ if (isa(this->getType())) {
return this->getType();
- else if (const auto *ECD = this->getEnumConstantDecl())
-return Ctx.getTypeDeclType(cast(ECD->getDeclContext()));
+ } else if (const auto *ECD = this->getEnumConstantDecl()) {
+const auto *ED = cast(ECD->getDeclContext());
+if (ED->isCompleteDefinition())
+ return Ctx.getTypeDeclType(ED);
+ }
return this->getType();
}
diff --git a/clang/test/Sema/warn-compare-enum-types-mismatch.c
b/clang/test/Sema/warn-compare-enum-types-mismatch.c
index 2b72aae16b977a..1094a6972e778d 100644
--- a/clang/test/Sema/warn-compare-enum-types-mismatch.c
+++ b/clang/test/Sema/warn-compare-enum-types-mismatch.c
@@ -6,7 +6,9 @@ typedef enum EnumA {
} EnumA;
enum EnumB {
- B
+ B,
+ B1 = 1,
+ B2 = A == B1
};
enum {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
