[PATCH] D53921: Compound literals, enums, et al require const expr

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

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

Thanks, other than the compound literal part I think this is all fine. I'm happy for you to go ahead with this as-is; we can rearrange how we handle compound literals as a later change if you prefer.



================
Comment at: include/clang/AST/Expr.h:3073-3074
+      e = ice->getSubExpr();
+    else if (ConstantExpr *ce = dyn_cast<ConstantExpr>(e))
+      e = ce->getSubExpr();
+    else
----------------
Should we skip an arbitrary `FullExpr` here (and in the other `Ignore` functions below)?


================
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() &&
----------------
void wrote:
> rsmith wrote:
> > 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.)
> I thought the purpose of `ConstantExpr` is to specify those places where a constant expression is required. I.e., we can't have something like:
> 
> ```
> int z;
> foo y = (foo){z + 2};
> ```
> 
> In this case, `z + 2` would be wrapped by the `ConstantExpr` class. But in a function or module scope, then it would be fine:
> 
> ```
> void x(int z) {
>   foo y = (foo){z + 2};
> }
> ```
> 
> So `z + 2` wouldn't be wrapped. If I perform the wrapping in `CheckConvertedConstantExpr`, et al, then it doesn't seem like I have the context to say that it's a) a compound literal, and b) in file scope. So how can I correctly wrap it?
In the above example, I think we should have a `ConstantExpr` wrapping the entire initializer of `y`, because the whole thing is required to be a constant expression. Eg,

```
int z = 123;
```

... would *also* have a `ConstantExpr` wrapped around it -- indeed, in C, there should be a `ConstantExpr` around the initializer of all global variables, because they're all required to be initialized by constant expressions. So that's why I think the compound-literal part is a red herring.


================
Comment at: lib/Sema/SemaType.cpp:2236-2238
+  if (ArraySize && !CurContext->isFunctionOrMethod())
+    // A file-scoped array must have a constant array size.
+    ArraySize = new (Context) ConstantExpr(ArraySize);
----------------
As noted above, I'd prefer for `VerifyIntegerConstantExpression` to create this. But if we need to do it here, we should do it on the code path that creates a `ConstantArrayType`, not based on whether the array type appears at file scope.


================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:1286-1288
+      // Handled due to it being a wrapper class.
+      break;
+
----------------
Maybe just share the code for the two `FullExpr` cases?


Repository:
  rC Clang

https://reviews.llvm.org/D53921