Compound literals, enums, file-scoped arrays, etc. require their
initializers and size specifiers to be constant. Wrap the initializer
expressions in a ConstantExpr so that we can easily check for this later
on.
Details
Diff Detail
- Repository
- rC Clang
Event Timeline
lib/Sema/SemaDecl.cpp | ||
---|---|---|
16411 ↗ | (On Diff #171868) | probably don't need to adjust this line |
lib/Sema/SemaDecl.cpp | ||
---|---|---|
16411 ↗ | (On Diff #171868) | I kinda felt compelled to do it when I saw it. Became twitchy. :-) |
lib/AST/ExprConstant.cpp | ||
---|---|---|
5266–5268 | This shouldn't be necessary because you changed the CRTP base class ExprEvaluatorBase to do this already. | |
5741–5743 | Likewise here. | |
lib/Sema/SemaDecl.cpp | ||
16086–16094 ↗ | (On Diff #171976) | 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.) |
lib/Sema/SemaExpr.cpp | ||
5747–5748 | 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.) | |
5774 | (This approach is at least not entirely correct: compound literals are only required to have a constant initializer at file scope.) | |
lib/Sema/SemaType.cpp | ||
2181 | This call calls VerifyIntegerConstantExpression; I think we should be creating the ConstantExpr node within there when appropriate. |
lib/Sema/SemaDecl.cpp | ||
---|---|---|
16086–16094 ↗ | (On Diff #171976) | 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? |
lib/Sema/SemaExpr.cpp | ||
5747–5748 | I changed the code to wrap around here. But see my comment above. I think we might be thinking about ConstantExpr differently. |
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.
include/clang/AST/Expr.h | ||
---|---|---|
3073–3074 | Should we skip an arbitrary FullExpr here (and in the other Ignore functions below)? | |
lib/Sema/SemaDecl.cpp | ||
16086–16094 ↗ | (On Diff #171976) | 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. |
lib/Sema/SemaType.cpp | ||
2236–2238 | 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. | |
lib/StaticAnalyzer/Core/ExprEngine.cpp | ||
1286–1288 | Maybe just share the code for the two FullExpr cases? |
I'm using "arcanist" for the first time, and I don't want to mess things up. I'll address the comments here in the next patch.
Should we skip an arbitrary FullExpr here (and in the other Ignore functions below)?