[PATCH] D53921: Compound literals, global array decls, and enums require constant inits

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: lib/AST/ExprConstant.cpp:5266-5268
+  bool VisitConstantExpr(const ConstantExpr *E) {
+    return Visit(E->getSubExpr());
+  }
----------------
This shouldn't be necessary because you changed the CRTP base class `ExprEvaluatorBase` to do this already.


================
Comment at: lib/AST/ExprConstant.cpp:5744-5746
+  bool VisitConstantExpr(const ConstantExpr *E) {
+    return evaluateLValue(E->getSubExpr(), Result);
+  }
----------------
Likewise here.


================
Comment at: lib/Sema/SemaDecl.cpp:16086-16094
           CheckConvertedConstantExpression(Val, EltTy, EnumVal,
                                            CCEK_Enumerator);
         if (Converted.isInvalid())
           Val = nullptr;
         else
           Val = Converted.get();
       } else if (!Val->isValueDependent() &&
----------------
I think it would make sense to create the `ConstantExpr` wrapper node in `CheckConvertedConstantExpr` / `VerifyIntegerConstantExpr`. (That's also where it'd make sense to cache the evaluated value on the wrapper node once we start doing so.)


================
Comment at: lib/Sema/SemaExpr.cpp:5752
         !literalType->isDependentType()) // C99 6.5.2.5p3
       if (CheckForConstantInitializer(LiteralExpr, literalType))
         return ExprError();
----------------
Can we create the `ConstantExpr` in here instead (assuming we need it at all)?

(The reason it's not clear to me that we need it at all is that file-scope compound literals can only appear in contexts where a constant expression is required *anyway* -- be they in array bounds, enumerators, or the initializer of a global variable.)


================
Comment at: lib/Sema/SemaExpr.cpp:5789
+                                                VK, LiteralExpr, isFileScope);
+  return MaybeBindToTemporary(new (Context) ConstantExpr(CLE));
 }
----------------
(This approach is at least not entirely correct: compound literals are only required to have a constant initializer at file scope.)


================
Comment at: lib/Sema/SemaType.cpp:2181
               !T->isConstantSizeType()) ||
              isArraySizeVLA(*this, ArraySize, ConstVal)) {
     // Even in C++11, don't allow contextual conversions in the array bound
----------------
This call calls `VerifyIntegerConstantExpression`; I think we should be creating the `ConstantExpr` node within there when appropriate.


Repository:
  rC Clang

https://reviews.llvm.org/D53921