rsmith added a comment.
This needs testcases (the one from your summary plus the ones in my comments
above would be good).
================
Comment at: lib/AST/ExprConstant.cpp:2622
// Next subobject is an array element.
- const ConstantArrayType *CAT = Info.Ctx.getAsConstantArrayType(ObjType);
- assert(CAT && "vla in literal type?");
+ uint64_t actualIndex;
+ const ArrayType *AT = Info.Ctx.getAsArrayType(ObjType); // non-null by
assumption
----------------
I think this should be named `ArrayBound` or similar.
================
Comment at: lib/AST/ExprConstant.cpp:2625-2626
+ if (I == 0) {
+ /* we're dealing with the complete array object, which may have been
declared
+ without a bound */
+ actualIndex = Sub.MostDerivedArraySize;
----------------
Use line comments, start with capital letter, end with period, please.
================
Comment at: lib/AST/ExprConstant.cpp:2627
+ without a bound */
+ actualIndex = Sub.MostDerivedArraySize;
+ assert(Sub.MostDerivedIsArrayElement && "Complete object is an array,
but isn't?");
----------------
This will not be the correct bound in general. This field is the array size of
the *innermost* non-base-class subobject described by the designator, not the
outermost one.
You could compute the correct bound here by going back to the `ValueDecl*` in
the `CompleteObject` and checking its most recent declaration, but I think it
would be preferable to do that when computing the type in `findCompleteObject`
instead.
(It might seem possible to map to the most recent `VarDecl` when evaluating the
`DeclRefExpr` instead, but that actually won't work in general, for cases like:
```
extern int arr[];
constexpr int *p = arr; // computes APValue referring to first declaration of
'arr'
int arr[3];
constexpr int *q = p + 1; // should be accepted
```
... but `findCompleteObject` will happen late enough.)
================
Comment at: lib/AST/ExprConstant.cpp:5652-5653
return false;
- } else {
+ }
+ else {
Result.set(SubExpr, Info.CurrentCall->Index);
----------------
Per LLVM coding style, `else` goes on the same line as `}`.
================
Comment at: lib/AST/ExprConstant.cpp:5666
+ else if (auto decl = Result.Base.dyn_cast<ValueDecl const*>()) {
+ // make sure to consider the completed type.
+ if (auto CAT = Info.Ctx.getAsConstantArrayType(cast<ValueDecl const>(
----------------
Comments should start with a capital letter.
================
Comment at: lib/AST/ExprConstant.cpp:5670-5673
+ else {
+ Result.Designator.setInvalid();
+ CCEDiag(SubExpr, diag::note_constexpr_array_unknown_bound_decay);
+ }
----------------
Please suppress this diagnostic when
`Info.checkingPotentialConstantExpression()`, since the expression is still
potentially a constant expression. Example:
```
extern int arr[];
constexpr int *f() { return arr + 3; }
int arr[5];
constexpr int *p = f();
```
Here, when we check that the body of `f()` is valid, we need to ignore the fact
that we don't know the array bound, since it could become known later.
================
Comment at: lib/AST/ExprConstant.cpp:5676
else
Result.Designator.setInvalid();
return true;
----------------
We should produce a `CCEDiag` along this path too. (This happens for VLAs.)
https://reviews.llvm.org/D32372
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
