[PATCH] D131874: [Clang] Tighten restrictions on enum out of range diagnostic to avoid constant initialization

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

Thanks! It seems like this now finally works as it should again, with the code 
I regularly build.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131874

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131874: [Clang] Tighten restrictions on enum out of range diagnostic to avoid constant initialization

[Shafik Yaghmour via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG8de51375f12d: [Clang] Tighten restrictions on enum out of 
range diagnostic to avoid constant… (authored by shafik).
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131874

Files:
  clang/lib/AST/ExprConstant.cpp
  clang/test/SemaCXX/constant-expression-cxx11.cpp
  clang/test/SemaCXX/cxx2a-consteval.cpp


Index: clang/test/SemaCXX/cxx2a-consteval.cpp
===
--- clang/test/SemaCXX/cxx2a-consteval.cpp
+++ clang/test/SemaCXX/cxx2a-consteval.cpp
@@ -865,3 +865,15 @@
 }
 
 } // namespace multiple_default_constructors
+
+namespace GH50055 {
+enum E {e1=0, e2=1};
+consteval int testDefaultArgForParam(E eParam = (E)-1) {
+// expected-error@-1 {{integer value -1 is outside the valid range of values 
[0, 1] for this enumeration type}}
+  return (int)eParam;
+}
+
+int test() {
+  return testDefaultArgForParam() + testDefaultArgForParam((E)1);
+}
+}
Index: clang/test/SemaCXX/constant-expression-cxx11.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx11.cpp
+++ clang/test/SemaCXX/constant-expression-cxx11.cpp
@@ -2419,6 +2419,13 @@
 
 enum EMaxInt {emaxint1=-1, emaxint2=__INT_MAX__};
 
+enum NumberType {};
+
+E2 testDefaultArgForParam(E2 e2Param = (E2)-1) { // ok, not a constant 
expression context
+  E2 e2LocalInit = e2Param; // ok, not a constant expression context
+  return e2LocalInit;
+}
+
 void testValueInRangeOfEnumerationValues() {
   constexpr E1 x1 = static_cast(-8);
   constexpr E1 x2 = static_cast(8);
@@ -2454,6 +2461,8 @@
   constexpr EMaxInt x19 = static_cast(__INT_MAX__-1);
   constexpr EMaxInt x20 = static_cast((long)__INT_MAX__+1);
   // expected-error@-1 {{integer value 2147483648 is outside the valid range 
of values [-2147483648, 2147483647] for this enumeration type}}
+
+  const NumberType neg_one = (NumberType) ((NumberType) 0 - (NumberType) 1); 
// ok, not a constant expression context
 }
 
 enum SortOrder {
@@ -2470,3 +2479,8 @@
 return;
 }
 }
+
+GH50055::E2 GlobalInitNotCE1 = (GH50055::E2)-1; // ok, not a constant 
expression context
+GH50055::E2 GlobalInitNotCE2 = GH50055::testDefaultArgForParam(); // ok, not a 
constant expression context
+constexpr GH50055::E2 GlobalInitCE = (GH50055::E2)-1;
+// expected-error@-1 {{integer value -1 is outside the valid range of values 
[0, 7] for this enumeration type}}
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -13536,6 +13536,18 @@
 if (Info.Ctx.getLangOpts().CPlusPlus && Info.InConstantContext &&
 Info.EvalMode == EvalInfo::EM_ConstantExpression &&
 DestType->isEnumeralType()) {
+
+  bool ConstexprVar = true;
+
+  // We know if we are here that we are in a context that we might require
+  // a constant expression or a context that requires a constant
+  // value. But if we are initializing a value we don't know if it is a
+  // constexpr variable or not. We can check the EvaluatingDecl to 
determine
+  // if it constexpr or not. If not then we don't want to emit a 
diagnostic.
+  if (const auto *VD = dyn_cast_or_null(
+  Info.EvaluatingDecl.dyn_cast()))
+ConstexprVar = VD->isConstexpr();
+
   const EnumType *ET = dyn_cast(DestType.getCanonicalType());
   const EnumDecl *ED = ET->getDecl();
   // Check that the value is within the range of the enumeration values.
@@ -13555,13 +13567,14 @@
 ED->getValueRange(Max, Min);
 --Max;
 
-if (ED->getNumNegativeBits() &&
+if (ED->getNumNegativeBits() && ConstexprVar &&
 (Max.slt(Result.getInt().getSExtValue()) ||
  Min.sgt(Result.getInt().getSExtValue(
-  Info.Ctx.getDiagnostics().Report(E->getExprLoc(),
-   
diag::warn_constexpr_unscoped_enum_out_of_range)
-  << llvm::toString(Result.getInt(),10) << Min.getSExtValue() << 
Max.getSExtValue();
-else if (!ED->getNumNegativeBits() &&
+  Info.Ctx.getDiagnostics().Report(
+  E->getExprLoc(), diag::warn_constexpr_unscoped_enum_out_of_range)
+  << llvm::toString(Result.getInt(), 10) << Min.getSExtValue()
+  << Max.getSExtValue();
+else if (!ED->getNumNegativeBits() && ConstexprVar &&
  Max.ult(Result.getInt().getZExtValue()))
   Info.Ctx.getDiagnostics().Report(E->getExprLoc(),

diag::warn_constexpr_unscoped_enum_out_of_range)


Index: clang/test/SemaCXX/cxx2a-consteval.cpp
===
--- clang/test/SemaCXX/cxx2a-consteval.cpp
+++ 

[PATCH] D131874: [Clang] Tighten restrictions on enum out of range diagnostic to avoid constant initialization

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM!


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

https://reviews.llvm.org/D131874

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131874: [Clang] Tighten restrictions on enum out of range diagnostic to avoid constant initialization

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik updated this revision to Diff 453389.
shafik marked an inline comment as done.
shafik added a comment.

- Added `consteval` test with default parameters


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

https://reviews.llvm.org/D131874

Files:
  clang/lib/AST/ExprConstant.cpp
  clang/test/SemaCXX/constant-expression-cxx11.cpp
  clang/test/SemaCXX/cxx2a-consteval.cpp


Index: clang/test/SemaCXX/cxx2a-consteval.cpp
===
--- clang/test/SemaCXX/cxx2a-consteval.cpp
+++ clang/test/SemaCXX/cxx2a-consteval.cpp
@@ -865,3 +865,15 @@
 }
 
 } // namespace multiple_default_constructors
+
+namespace GH50055 {
+enum E {e1=0, e2=1};
+consteval int testDefaultArgForParam(E eParam = (E)-1) {
+// expected-error@-1 {{integer value -1 is outside the valid range of values 
[0, 1] for this enumeration type}}
+  return (int)eParam;
+}
+
+int test() {
+  return testDefaultArgForParam() + testDefaultArgForParam((E)1);
+}
+}
Index: clang/test/SemaCXX/constant-expression-cxx11.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx11.cpp
+++ clang/test/SemaCXX/constant-expression-cxx11.cpp
@@ -2419,6 +2419,13 @@
 
 enum EMaxInt {emaxint1=-1, emaxint2=__INT_MAX__};
 
+enum NumberType {};
+
+E2 testDefaultArgForParam(E2 e2Param = (E2)-1) { // ok, not a constant 
expression context
+  E2 e2LocalInit = e2Param; // ok, not a constant expression context
+  return e2LocalInit;
+}
+
 void testValueInRangeOfEnumerationValues() {
   constexpr E1 x1 = static_cast(-8);
   constexpr E1 x2 = static_cast(8);
@@ -2454,6 +2461,8 @@
   constexpr EMaxInt x19 = static_cast(__INT_MAX__-1);
   constexpr EMaxInt x20 = static_cast((long)__INT_MAX__+1);
   // expected-error@-1 {{integer value 2147483648 is outside the valid range 
of values [-2147483648, 2147483647] for this enumeration type}}
+
+  const NumberType neg_one = (NumberType) ((NumberType) 0 - (NumberType) 1); 
// ok, not a constant expression context
 }
 
 enum SortOrder {
@@ -2470,3 +2479,8 @@
 return;
 }
 }
+
+GH50055::E2 GlobalInitNotCE1 = (GH50055::E2)-1; // ok, not a constant 
expression context
+GH50055::E2 GlobalInitNotCE2 = GH50055::testDefaultArgForParam(); // ok, not a 
constant expression context
+constexpr GH50055::E2 GlobalInitCE = (GH50055::E2)-1;
+// expected-error@-1 {{integer value -1 is outside the valid range of values 
[0, 7] for this enumeration type}}
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -13536,6 +13536,18 @@
 if (Info.Ctx.getLangOpts().CPlusPlus && Info.InConstantContext &&
 Info.EvalMode == EvalInfo::EM_ConstantExpression &&
 DestType->isEnumeralType()) {
+
+  bool ConstexprVar = true;
+
+  // We know if we are here that we are in a context that we might require
+  // a constant expression or a context that requires a constant
+  // value. But if we are initializing a value we don't know if it is a
+  // constexpr variable or not. We can check the EvaluatingDecl to 
determine
+  // if it constexpr or not. If not then we don't want to emit a 
diagnostic.
+  if (const auto *VD = dyn_cast_or_null(
+  Info.EvaluatingDecl.dyn_cast()))
+ConstexprVar = VD->isConstexpr();
+
   const EnumType *ET = dyn_cast(DestType.getCanonicalType());
   const EnumDecl *ED = ET->getDecl();
   // Check that the value is within the range of the enumeration values.
@@ -13555,13 +13567,14 @@
 ED->getValueRange(Max, Min);
 --Max;
 
-if (ED->getNumNegativeBits() &&
+if (ED->getNumNegativeBits() && ConstexprVar &&
 (Max.slt(Result.getInt().getSExtValue()) ||
  Min.sgt(Result.getInt().getSExtValue(
-  Info.Ctx.getDiagnostics().Report(E->getExprLoc(),
-   
diag::warn_constexpr_unscoped_enum_out_of_range)
-  << llvm::toString(Result.getInt(),10) << Min.getSExtValue() << 
Max.getSExtValue();
-else if (!ED->getNumNegativeBits() &&
+  Info.Ctx.getDiagnostics().Report(
+  E->getExprLoc(), diag::warn_constexpr_unscoped_enum_out_of_range)
+  << llvm::toString(Result.getInt(), 10) << Min.getSExtValue()
+  << Max.getSExtValue();
+else if (!ED->getNumNegativeBits() && ConstexprVar &&
  Max.ult(Result.getInt().getZExtValue()))
   Info.Ctx.getDiagnostics().Report(E->getExprLoc(),

diag::warn_constexpr_unscoped_enum_out_of_range)


Index: clang/test/SemaCXX/cxx2a-consteval.cpp
===
--- clang/test/SemaCXX/cxx2a-consteval.cpp
+++ clang/test/SemaCXX/cxx2a-consteval.cpp
@@ -865,3 +865,15 @@
 }
 
 } // namespace multiple_default_constructors
+

[PATCH] D131874: [Clang] Tighten restrictions on enum out of range diagnostic to avoid constant initialization

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik updated this revision to Diff 453350.
shafik marked 2 inline comments as done.
shafik added a comment.

- Simplify condition to set `ConstexprVar` even more


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

https://reviews.llvm.org/D131874

Files:
  clang/lib/AST/ExprConstant.cpp
  clang/test/SemaCXX/constant-expression-cxx11.cpp


Index: clang/test/SemaCXX/constant-expression-cxx11.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx11.cpp
+++ clang/test/SemaCXX/constant-expression-cxx11.cpp
@@ -2419,6 +2419,13 @@
 
 enum EMaxInt {emaxint1=-1, emaxint2=__INT_MAX__};
 
+enum NumberType {};
+
+E2 testDefaultArgForParam(E2 e2Param = (E2)-1) { // ok, not a constant 
expression context
+  E2 e2LocalInit = e2Param; // ok, not a constant expression context
+  return e2LocalInit;
+}
+
 void testValueInRangeOfEnumerationValues() {
   constexpr E1 x1 = static_cast(-8);
   constexpr E1 x2 = static_cast(8);
@@ -2454,6 +2461,8 @@
   constexpr EMaxInt x19 = static_cast(__INT_MAX__-1);
   constexpr EMaxInt x20 = static_cast((long)__INT_MAX__+1);
   // expected-error@-1 {{integer value 2147483648 is outside the valid range 
of values [-2147483648, 2147483647] for this enumeration type}}
+
+  const NumberType neg_one = (NumberType) ((NumberType) 0 - (NumberType) 1); 
// ok, not a constant expression context
 }
 
 enum SortOrder {
@@ -2470,3 +2479,8 @@
 return;
 }
 }
+
+GH50055::E2 GlobalInitNotCE1 = (GH50055::E2)-1; // ok, not a constant 
expression context
+GH50055::E2 GlobalInitNotCE2 = GH50055::testDefaultArgForParam(); // ok, not a 
constant expression context
+constexpr GH50055::E2 GlobalInitCE = (GH50055::E2)-1;
+// expected-error@-1 {{integer value -1 is outside the valid range of values 
[0, 7] for this enumeration type}}
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -13536,6 +13536,18 @@
 if (Info.Ctx.getLangOpts().CPlusPlus && Info.InConstantContext &&
 Info.EvalMode == EvalInfo::EM_ConstantExpression &&
 DestType->isEnumeralType()) {
+
+  bool ConstexprVar = true;
+
+  // We know if we are here that we are in a context that we might require
+  // a constant expression or a context that requires a constant
+  // value. But if we are initializing a value we don't know if it is a
+  // constexpr variable or not. We can check the EvaluatingDecl to 
determine
+  // if it constexpr or not. If not then we don't want to emit a 
diagnostic.
+  if (const auto *VD = dyn_cast_or_null(
+  Info.EvaluatingDecl.dyn_cast()))
+ConstexprVar = VD->isConstexpr();
+
   const EnumType *ET = dyn_cast(DestType.getCanonicalType());
   const EnumDecl *ED = ET->getDecl();
   // Check that the value is within the range of the enumeration values.
@@ -13555,13 +13567,14 @@
 ED->getValueRange(Max, Min);
 --Max;
 
-if (ED->getNumNegativeBits() &&
+if (ED->getNumNegativeBits() && ConstexprVar &&
 (Max.slt(Result.getInt().getSExtValue()) ||
  Min.sgt(Result.getInt().getSExtValue(
-  Info.Ctx.getDiagnostics().Report(E->getExprLoc(),
-   
diag::warn_constexpr_unscoped_enum_out_of_range)
-  << llvm::toString(Result.getInt(),10) << Min.getSExtValue() << 
Max.getSExtValue();
-else if (!ED->getNumNegativeBits() &&
+  Info.Ctx.getDiagnostics().Report(
+  E->getExprLoc(), diag::warn_constexpr_unscoped_enum_out_of_range)
+  << llvm::toString(Result.getInt(), 10) << Min.getSExtValue()
+  << Max.getSExtValue();
+else if (!ED->getNumNegativeBits() && ConstexprVar &&
  Max.ult(Result.getInt().getZExtValue()))
   Info.Ctx.getDiagnostics().Report(E->getExprLoc(),

diag::warn_constexpr_unscoped_enum_out_of_range)


Index: clang/test/SemaCXX/constant-expression-cxx11.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx11.cpp
+++ clang/test/SemaCXX/constant-expression-cxx11.cpp
@@ -2419,6 +2419,13 @@
 
 enum EMaxInt {emaxint1=-1, emaxint2=__INT_MAX__};
 
+enum NumberType {};
+
+E2 testDefaultArgForParam(E2 e2Param = (E2)-1) { // ok, not a constant expression context
+  E2 e2LocalInit = e2Param; // ok, not a constant expression context
+  return e2LocalInit;
+}
+
 void testValueInRangeOfEnumerationValues() {
   constexpr E1 x1 = static_cast(-8);
   constexpr E1 x2 = static_cast(8);
@@ -2454,6 +2461,8 @@
   constexpr EMaxInt x19 = static_cast(__INT_MAX__-1);
   constexpr EMaxInt x20 = static_cast((long)__INT_MAX__+1);
   // expected-error@-1 {{integer value 2147483648 is outside the valid range of values [-2147483648, 2147483647] for this enumeration type}}
+
+ 

[PATCH] D131874: [Clang] Tighten restrictions on enum out of range diagnostic to avoid constant initialization

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Precommit CI failures are unrelated to this patch.




Comment at: clang/lib/AST/ExprConstant.cpp:13547-13550
+  if (const auto *VD = 
dyn_cast_or_null(Info.EvaluatingDecl.dyn_cast())) {
+if (VD && !VD->isConstexpr())
+  ConstexprVar = false;
+  }

You already know that `VD` is nonnull, so no need for that check, and we can 
remove the predicate entirely because you're setting `ConstexprVar` based on 
`isConstexpr()` anyway.


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

https://reviews.llvm.org/D131874

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131874: [Clang] Tighten restrictions on enum out of range diagnostic to avoid constant initialization

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

I confirmed that this fixes all remaining issues on our end. Thank you!


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

https://reviews.llvm.org/D131874

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131874: [Clang] Tighten restrictions on enum out of range diagnostic to avoid constant initialization

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik updated this revision to Diff 453062.
shafik marked an inline comment as done.
shafik added a comment.

- Addressing comments on casting of `EvaluatingDecl`


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

https://reviews.llvm.org/D131874

Files:
  clang/lib/AST/ExprConstant.cpp
  clang/test/SemaCXX/constant-expression-cxx11.cpp


Index: clang/test/SemaCXX/constant-expression-cxx11.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx11.cpp
+++ clang/test/SemaCXX/constant-expression-cxx11.cpp
@@ -2419,6 +2419,13 @@
 
 enum EMaxInt {emaxint1=-1, emaxint2=__INT_MAX__};
 
+enum NumberType {};
+
+E2 testDefaultArgForParam(E2 e2Param = (E2)-1) { // ok, not a constant 
expression context
+  E2 e2LocalInit = e2Param; // ok, not a constant expression context
+  return e2LocalInit;
+}
+
 void testValueInRangeOfEnumerationValues() {
   constexpr E1 x1 = static_cast(-8);
   constexpr E1 x2 = static_cast(8);
@@ -2454,6 +2461,8 @@
   constexpr EMaxInt x19 = static_cast(__INT_MAX__-1);
   constexpr EMaxInt x20 = static_cast((long)__INT_MAX__+1);
   // expected-error@-1 {{integer value 2147483648 is outside the valid range 
of values [-2147483648, 2147483647] for this enumeration type}}
+
+  const NumberType neg_one = (NumberType) ((NumberType) 0 - (NumberType) 1); 
// ok, not a constant expression context
 }
 
 enum SortOrder {
@@ -2470,3 +2479,8 @@
 return;
 }
 }
+
+GH50055::E2 GlobalInitNotCE1 = (GH50055::E2)-1; // ok, not a constant 
expression context
+GH50055::E2 GlobalInitNotCE2 = GH50055::testDefaultArgForParam(); // ok, not a 
constant expression context
+constexpr GH50055::E2 GlobalInitCE = (GH50055::E2)-1;
+// expected-error@-1 {{integer value -1 is outside the valid range of values 
[0, 7] for this enumeration type}}
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -13536,6 +13536,19 @@
 if (Info.Ctx.getLangOpts().CPlusPlus && Info.InConstantContext &&
 Info.EvalMode == EvalInfo::EM_ConstantExpression &&
 DestType->isEnumeralType()) {
+
+  bool ConstexprVar = true;
+
+  // We know if we are here that we are in a context that we might require
+  // a constant expression or a context that requires a constant
+  // value. But if we are initializing a value we don't know if it is a
+  // constexpr variable or not. We can check the EvaluatingDecl to 
determine
+  // if it constexpr or not. If not then we don't want to emit a 
diagnostic.
+  if (const auto *VD = 
dyn_cast_or_null(Info.EvaluatingDecl.dyn_cast())) {
+if (VD && !VD->isConstexpr())
+  ConstexprVar = false;
+  }
+
   const EnumType *ET = dyn_cast(DestType.getCanonicalType());
   const EnumDecl *ED = ET->getDecl();
   // Check that the value is within the range of the enumeration values.
@@ -13555,13 +13568,14 @@
 ED->getValueRange(Max, Min);
 --Max;
 
-if (ED->getNumNegativeBits() &&
+if (ED->getNumNegativeBits() && ConstexprVar &&
 (Max.slt(Result.getInt().getSExtValue()) ||
  Min.sgt(Result.getInt().getSExtValue(
-  Info.Ctx.getDiagnostics().Report(E->getExprLoc(),
-   
diag::warn_constexpr_unscoped_enum_out_of_range)
-  << llvm::toString(Result.getInt(),10) << Min.getSExtValue() << 
Max.getSExtValue();
-else if (!ED->getNumNegativeBits() &&
+  Info.Ctx.getDiagnostics().Report(
+  E->getExprLoc(), diag::warn_constexpr_unscoped_enum_out_of_range)
+  << llvm::toString(Result.getInt(), 10) << Min.getSExtValue()
+  << Max.getSExtValue();
+else if (!ED->getNumNegativeBits() && ConstexprVar &&
  Max.ult(Result.getInt().getZExtValue()))
   Info.Ctx.getDiagnostics().Report(E->getExprLoc(),

diag::warn_constexpr_unscoped_enum_out_of_range)


Index: clang/test/SemaCXX/constant-expression-cxx11.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx11.cpp
+++ clang/test/SemaCXX/constant-expression-cxx11.cpp
@@ -2419,6 +2419,13 @@
 
 enum EMaxInt {emaxint1=-1, emaxint2=__INT_MAX__};
 
+enum NumberType {};
+
+E2 testDefaultArgForParam(E2 e2Param = (E2)-1) { // ok, not a constant expression context
+  E2 e2LocalInit = e2Param; // ok, not a constant expression context
+  return e2LocalInit;
+}
+
 void testValueInRangeOfEnumerationValues() {
   constexpr E1 x1 = static_cast(-8);
   constexpr E1 x2 = static_cast(8);
@@ -2454,6 +2461,8 @@
   constexpr EMaxInt x19 = static_cast(__INT_MAX__-1);
   constexpr EMaxInt x20 = static_cast((long)__INT_MAX__+1);
   // expected-error@-1 {{integer value 2147483648 is outside the valid range of values [-2147483648, 2147483647] for 

[PATCH] D131874: [Clang] Tighten restrictions on enum out of range diagnostic to avoid constant initialization

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:13547-13551
+  if (auto ValD = Info.EvaluatingDecl.dyn_cast()) {
+const VarDecl *VD = dyn_cast_or_null(ValD);
+if (VD && !VD->isConstexpr())
+  NotConstexprVar = true;
+  }

shafik wrote:
> shafik wrote:
> > aaron.ballman wrote:
> > > This seems to be equivalent unless I'm misunderstanding something about 
> > > the member dyn_cast.
> > I think the problem is that `PointerUnion` requires that it be one of the 
> > static types it was defined with and in this case that is `const ValueDecl 
> > *, const Expr *` but maybe I am missing something.
> It looks like the big difference is that in `doCastIfPossible(...)` we end up 
> calling `Self::isPossible(f)` which ends up in `PointerUnion::isPossible...)` 
> which uses `FirstIndexOfType` and if the template arguments don't contain the 
> type then it fail.
Whelp, TIL!

Then I guess I'd go with:

`if (const auto *VD = 
dyn_cast_or_null(Info.EvaluatingDecl.dyn_cast))`


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

https://reviews.llvm.org/D131874

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131874: [Clang] Tighten restrictions on enum out of range diagnostic to avoid constant initialization

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:13547-13551
+  if (auto ValD = Info.EvaluatingDecl.dyn_cast()) {
+const VarDecl *VD = dyn_cast_or_null(ValD);
+if (VD && !VD->isConstexpr())
+  NotConstexprVar = true;
+  }

shafik wrote:
> aaron.ballman wrote:
> > This seems to be equivalent unless I'm misunderstanding something about the 
> > member dyn_cast.
> I think the problem is that `PointerUnion` requires that it be one of the 
> static types it was defined with and in this case that is `const ValueDecl *, 
> const Expr *` but maybe I am missing something.
It looks like the big difference is that in `doCastIfPossible(...)` we end up 
calling `Self::isPossible(f)` which ends up in `PointerUnion::isPossible...)` 
which uses `FirstIndexOfType` and if the template arguments don't contain the 
type then it fail.


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

https://reviews.llvm.org/D131874

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131874: [Clang] Tighten restrictions on enum out of range diagnostic to avoid constant initialization

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:13547-13551
+  if (auto ValD = Info.EvaluatingDecl.dyn_cast()) {
+const VarDecl *VD = dyn_cast_or_null(ValD);
+if (VD && !VD->isConstexpr())
+  NotConstexprVar = true;
+  }

aaron.ballman wrote:
> This seems to be equivalent unless I'm misunderstanding something about the 
> member dyn_cast.
I think the problem is that `PointerUnion` requires that it be one of the 
static types it was defined with and in this case that is `const ValueDecl *, 
const Expr *` but maybe I am missing something.



Comment at: clang/lib/AST/ExprConstant.cpp:13572-13575
 if (ED->getNumNegativeBits() &&
 (Max.slt(Result.getInt().getSExtValue()) ||
- Min.sgt(Result.getInt().getSExtValue(
-  Info.Ctx.getDiagnostics().Report(E->getExprLoc(),
-   
diag::warn_constexpr_unscoped_enum_out_of_range)
-  << llvm::toString(Result.getInt(),10) << Min.getSExtValue() << 
Max.getSExtValue();
+ Min.sgt(Result.getInt().getSExtValue())) &&
+!NotConstexprVar)

aaron.ballman wrote:
> Might as well test the easy condition before doing more work to get values 
> and compare them.
> 
> I think we should rename `NotConstexprVar` to `ConstexprVar` so that we don't 
> need the double negation here. WDYT?
Makes sense.



Comment at: clang/test/SemaCXX/constant-expression-cxx11.cpp:2424-2427
+E2 testDefaultArgForParam(E2 e2Param = (E2)-1) { // ok, not a constant 
expression context
+  E2 e2LocalInit = e2Param; // ok, not a constant expression context
+  return e2LocalInit;
+}

aaron.ballman wrote:
> Do you think it's worth it to add this as a test as well?
> ```
> consteval void testDefaultArgForParam2(E2 e2Param = (E2)-1) {
> }
> 
> void test() {
>   testDefaultArgForParam2(); // Make sure we get the error
>   testDefaultArgForParam2((E2)0); // Make sure we don't get the error
> }
> ```
It is a good idea but the diagnostic does not point to the line in `test()` 
which is unfortunate. 


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

https://reviews.llvm.org/D131874

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131874: [Clang] Tighten restrictions on enum out of range diagnostic to avoid constant initialization

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik updated this revision to Diff 452775.
shafik marked an inline comment as done.
shafik added a comment.

- Changed `NotConstexprVar` to `ConstExprVar`
- Moved checking of `ConstexprVar` up so that we do the less expensive check 
first


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

https://reviews.llvm.org/D131874

Files:
  clang/lib/AST/ExprConstant.cpp
  clang/test/SemaCXX/constant-expression-cxx11.cpp


Index: clang/test/SemaCXX/constant-expression-cxx11.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx11.cpp
+++ clang/test/SemaCXX/constant-expression-cxx11.cpp
@@ -2419,6 +2419,13 @@
 
 enum EMaxInt {emaxint1=-1, emaxint2=__INT_MAX__};
 
+enum NumberType {};
+
+E2 testDefaultArgForParam(E2 e2Param = (E2)-1) { // ok, not a constant 
expression context
+  E2 e2LocalInit = e2Param; // ok, not a constant expression context
+  return e2LocalInit;
+}
+
 void testValueInRangeOfEnumerationValues() {
   constexpr E1 x1 = static_cast(-8);
   constexpr E1 x2 = static_cast(8);
@@ -2454,6 +2461,8 @@
   constexpr EMaxInt x19 = static_cast(__INT_MAX__-1);
   constexpr EMaxInt x20 = static_cast((long)__INT_MAX__+1);
   // expected-error@-1 {{integer value 2147483648 is outside the valid range 
of values [-2147483648, 2147483647] for this enumeration type}}
+
+  const NumberType neg_one = (NumberType) ((NumberType) 0 - (NumberType) 1); 
// ok, not a constant expression context
 }
 
 enum SortOrder {
@@ -2470,3 +2479,8 @@
 return;
 }
 }
+
+GH50055::E2 GlobalInitNotCE1 = (GH50055::E2)-1; // ok, not a constant 
expression context
+GH50055::E2 GlobalInitNotCE2 = GH50055::testDefaultArgForParam(); // ok, not a 
constant expression context
+constexpr GH50055::E2 GlobalInitCE = (GH50055::E2)-1;
+// expected-error@-1 {{integer value -1 is outside the valid range of values 
[0, 7] for this enumeration type}}
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -13536,6 +13536,20 @@
 if (Info.Ctx.getLangOpts().CPlusPlus && Info.InConstantContext &&
 Info.EvalMode == EvalInfo::EM_ConstantExpression &&
 DestType->isEnumeralType()) {
+
+  bool ConstexprVar = true;
+
+  // We know if we are here that we are in a context that we might require
+  // a constant expression or a context that requires a constant
+  // value. But if we are initializing a value we don't know if it is a
+  // constexpr variable or not. We can check the EvaluatingDecl to 
determine
+  // if it constexpr or not. If not then we don't want to emit a 
diagnostic.
+  if (auto ValD = Info.EvaluatingDecl.dyn_cast()) {
+const VarDecl *VD = dyn_cast_or_null(ValD);
+if (VD && !VD->isConstexpr())
+  ConstexprVar = false;
+  }
+
   const EnumType *ET = dyn_cast(DestType.getCanonicalType());
   const EnumDecl *ED = ET->getDecl();
   // Check that the value is within the range of the enumeration values.
@@ -13555,13 +13569,14 @@
 ED->getValueRange(Max, Min);
 --Max;
 
-if (ED->getNumNegativeBits() &&
+if (ED->getNumNegativeBits() && ConstexprVar &&
 (Max.slt(Result.getInt().getSExtValue()) ||
  Min.sgt(Result.getInt().getSExtValue(
-  Info.Ctx.getDiagnostics().Report(E->getExprLoc(),
-   
diag::warn_constexpr_unscoped_enum_out_of_range)
-  << llvm::toString(Result.getInt(),10) << Min.getSExtValue() << 
Max.getSExtValue();
-else if (!ED->getNumNegativeBits() &&
+  Info.Ctx.getDiagnostics().Report(
+  E->getExprLoc(), diag::warn_constexpr_unscoped_enum_out_of_range)
+  << llvm::toString(Result.getInt(), 10) << Min.getSExtValue()
+  << Max.getSExtValue();
+else if (!ED->getNumNegativeBits() && ConstexprVar &&
  Max.ult(Result.getInt().getZExtValue()))
   Info.Ctx.getDiagnostics().Report(E->getExprLoc(),

diag::warn_constexpr_unscoped_enum_out_of_range)


Index: clang/test/SemaCXX/constant-expression-cxx11.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx11.cpp
+++ clang/test/SemaCXX/constant-expression-cxx11.cpp
@@ -2419,6 +2419,13 @@
 
 enum EMaxInt {emaxint1=-1, emaxint2=__INT_MAX__};
 
+enum NumberType {};
+
+E2 testDefaultArgForParam(E2 e2Param = (E2)-1) { // ok, not a constant expression context
+  E2 e2LocalInit = e2Param; // ok, not a constant expression context
+  return e2LocalInit;
+}
+
 void testValueInRangeOfEnumerationValues() {
   constexpr E1 x1 = static_cast(-8);
   constexpr E1 x2 = static_cast(8);
@@ -2454,6 +2461,8 @@
   constexpr EMaxInt x19 = static_cast(__INT_MAX__-1);
   constexpr EMaxInt x20 = static_cast((long)__INT_MAX__+1);
   // 

[PATCH] D131874: [Clang] Tighten restrictions on enum out of range diagnostic to avoid constant initialization

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:13547-13551
+  if (auto ValD = Info.EvaluatingDecl.dyn_cast()) {
+const VarDecl *VD = dyn_cast_or_null(ValD);
+if (VD && !VD->isConstexpr())
+  NotConstexprVar = true;
+  }

This seems to be equivalent unless I'm misunderstanding something about the 
member dyn_cast.



Comment at: clang/lib/AST/ExprConstant.cpp:13572-13575
 if (ED->getNumNegativeBits() &&
 (Max.slt(Result.getInt().getSExtValue()) ||
- Min.sgt(Result.getInt().getSExtValue(
-  Info.Ctx.getDiagnostics().Report(E->getExprLoc(),
-   
diag::warn_constexpr_unscoped_enum_out_of_range)
-  << llvm::toString(Result.getInt(),10) << Min.getSExtValue() << 
Max.getSExtValue();
+ Min.sgt(Result.getInt().getSExtValue())) &&
+!NotConstexprVar)

Might as well test the easy condition before doing more work to get values and 
compare them.

I think we should rename `NotConstexprVar` to `ConstexprVar` so that we don't 
need the double negation here. WDYT?



Comment at: clang/test/SemaCXX/constant-expression-cxx11.cpp:2424-2427
+E2 testDefaultArgForParam(E2 e2Param = (E2)-1) { // ok, not a constant 
expression context
+  E2 e2LocalInit = e2Param; // ok, not a constant expression context
+  return e2LocalInit;
+}

Do you think it's worth it to add this as a test as well?
```
consteval void testDefaultArgForParam2(E2 e2Param = (E2)-1) {
}

void test() {
  testDefaultArgForParam2(); // Make sure we get the error
  testDefaultArgForParam2((E2)0); // Make sure we don't get the error
}
```


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

https://reviews.llvm.org/D131874

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131874: [Clang] Tighten restrictions on enum out of range diagnostic to avoid constant initialization

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

Thanks! With this, I think all my cases (that built before D131307 
) now build successfully again! (Can't say 
much about the code change itself, but I think it looks reasonable.)


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

https://reviews.llvm.org/D131874

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131874: [Clang] Tighten restrictions on enum out of range diagnostic to avoid constant initialization

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik created this revision.
shafik added reviewers: erichkeane, aaron.ballman, thakis, mstorsjo.
Herald added a project: All.
shafik requested review of this revision.

The restrictions added in D131704  were not 
sufficient to avoid all non-constant expression contexts. In particular 
constant initialization cases.

We need to check `EvaluatingDecl` to detect if the variable we are initializing 
is `constexpr` or not.

At this point it looks like this is the remaining case affecting various 
projects with this diagnostic.


https://reviews.llvm.org/D131874

Files:
  clang/lib/AST/ExprConstant.cpp
  clang/test/SemaCXX/constant-expression-cxx11.cpp


Index: clang/test/SemaCXX/constant-expression-cxx11.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx11.cpp
+++ clang/test/SemaCXX/constant-expression-cxx11.cpp
@@ -2419,6 +2419,13 @@
 
 enum EMaxInt {emaxint1=-1, emaxint2=__INT_MAX__};
 
+enum NumberType {};
+
+E2 testDefaultArgForParam(E2 e2Param = (E2)-1) { // ok, not a constant 
expression context
+  E2 e2LocalInit = e2Param; // ok, not a constant expression context
+  return e2LocalInit;
+}
+
 void testValueInRangeOfEnumerationValues() {
   constexpr E1 x1 = static_cast(-8);
   constexpr E1 x2 = static_cast(8);
@@ -2454,6 +2461,8 @@
   constexpr EMaxInt x19 = static_cast(__INT_MAX__-1);
   constexpr EMaxInt x20 = static_cast((long)__INT_MAX__+1);
   // expected-error@-1 {{integer value 2147483648 is outside the valid range 
of values [-2147483648, 2147483647] for this enumeration type}}
+
+  const NumberType neg_one = (NumberType) ((NumberType) 0 - (NumberType) 1); 
// ok, not a constant expression context
 }
 
 enum SortOrder {
@@ -2470,3 +2479,8 @@
 return;
 }
 }
+
+GH50055::E2 GlobalInitNotCE1 = (GH50055::E2)-1; // ok, not a constant 
expression context
+GH50055::E2 GlobalInitNotCE2 = GH50055::testDefaultArgForParam(); // ok, not a 
constant expression context
+constexpr GH50055::E2 GlobalInitCE = (GH50055::E2)-1;
+// expected-error@-1 {{integer value -1 is outside the valid range of values 
[0, 7] for this enumeration type}}
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -13536,6 +13536,20 @@
 if (Info.Ctx.getLangOpts().CPlusPlus && Info.InConstantContext &&
 Info.EvalMode == EvalInfo::EM_ConstantExpression &&
 DestType->isEnumeralType()) {
+
+  bool NotConstexprVar = false;
+
+  // We know if we are here that we are in a context that we might require
+  // a constant expression or a context that requires a constant
+  // value. But if we are initializing a value we don't know if it is a
+  // constexpr variable or not. We can check the EvaluatingDecl to 
determine
+  // if it constexpr or not. If not then we don't want to emit a 
diagnostic.
+  if (auto ValD = Info.EvaluatingDecl.dyn_cast()) {
+const VarDecl *VD = dyn_cast_or_null(ValD);
+if (VD && !VD->isConstexpr())
+  NotConstexprVar = true;
+  }
+
   const EnumType *ET = dyn_cast(DestType.getCanonicalType());
   const EnumDecl *ED = ET->getDecl();
   // Check that the value is within the range of the enumeration values.
@@ -13557,12 +13571,14 @@
 
 if (ED->getNumNegativeBits() &&
 (Max.slt(Result.getInt().getSExtValue()) ||
- Min.sgt(Result.getInt().getSExtValue(
-  Info.Ctx.getDiagnostics().Report(E->getExprLoc(),
-   
diag::warn_constexpr_unscoped_enum_out_of_range)
-  << llvm::toString(Result.getInt(),10) << Min.getSExtValue() << 
Max.getSExtValue();
+ Min.sgt(Result.getInt().getSExtValue())) &&
+!NotConstexprVar)
+  Info.Ctx.getDiagnostics().Report(
+  E->getExprLoc(), diag::warn_constexpr_unscoped_enum_out_of_range)
+  << llvm::toString(Result.getInt(), 10) << Min.getSExtValue()
+  << Max.getSExtValue();
 else if (!ED->getNumNegativeBits() &&
- Max.ult(Result.getInt().getZExtValue()))
+ Max.ult(Result.getInt().getZExtValue()) && !NotConstexprVar)
   Info.Ctx.getDiagnostics().Report(E->getExprLoc(),

diag::warn_constexpr_unscoped_enum_out_of_range)
<< llvm::toString(Result.getInt(),10) << Min.getZExtValue() << 
Max.getZExtValue();


Index: clang/test/SemaCXX/constant-expression-cxx11.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx11.cpp
+++ clang/test/SemaCXX/constant-expression-cxx11.cpp
@@ -2419,6 +2419,13 @@
 
 enum EMaxInt {emaxint1=-1, emaxint2=__INT_MAX__};
 
+enum NumberType {};
+
+E2 testDefaultArgForParam(E2 e2Param = (E2)-1) { // ok, not a constant expression context
+  E2 e2LocalInit