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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
