[PATCH] D32372: Arrays of unknown bound in constant expressions

[Richard Smith via Phabricator via cfe-commits]

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