[PATCH] D88236: [PR47636] Fix tryEmitPrivate to handle non-constantarraytypes

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

rsmith added inline comments.


================
Comment at: clang/test/CodeGenCXX/pr47636.cpp:2
+// RUN: %clang_cc1 -o - -emit-llvm -triple x86_64-linux-pc %s | FileCheck %s
+int(&&intu_rvref)[] {1,2,3,4};
+// CHECK: @_ZGR10intu_rvref_ = internal global [4 x i32] [i32 1, i32 2, i32 3, i32 4]
----------------
The AST we generate for this is:

```
`-VarDecl 0x106e7630 <<stdin>:1:1, col:29> col:7 intu_rvref 'int (&&)[4]' listinit
  `-ExprWithCleanups 0x106e7908 <col:21, col:29> 'int []':'int []' xvalue
    `-MaterializeTemporaryExpr 0x106e78a8 <col:21, col:29> 'int []':'int []' xvalue extended by Var 0x106e7630 'intu_rvref' 'int (&&)[4]'
      `-InitListExpr 0x106e77c0 <col:21, col:29> 'int [4]'
        |-IntegerLiteral 0x106e76e0 <col:22> 'int' 1
        |-IntegerLiteral 0x106e7700 <col:24> 'int' 2
        |-IntegerLiteral 0x106e7720 <col:26> 'int' 3
        `-IntegerLiteral 0x106e7740 <col:28> 'int' 4
```

... which looks surprising to me -- `MaterializeTemporaryExpr` corresponds to allocating storage for an object of type `T`, so `T` being incomplete is at least surprising, and it would seem more consistent to complete the type of the MTE in the same way we would complete the type of a corresponding `VarDecl`.

We also crash while processing

```
constexpr int f() { int (&&intu_rvref)[] {1,2,3,4}; return intu_rvref[0]; }
```

... due to the same unexpected AST representation.

I don't know if we would need this `CodeGen` change in other cases; it seems like if we ever supported constant initialization of a class with a flexible array member, we'd want to do something like this. For now at least, we refuse to constant-evaluate things such as `struct A { int n; int arr[]; } a = {1, 2, 3};` to an `APValue` and use the lowering-from-`Expr` path in `CGExprConstant` for such cases instead. So I suspect if we change the AST representation to use a complete type here, the code change here will be unreachable at least in the short term.

On the other hand, if we did choose to support constant initialization of flexible array members, it might be natural to treat objects of incomplete array type analogously to flexible array members, giving them an incomplete type whose size is determined from its initializer, and with that viewpoint the AST representation we're currently using wouldn't really be wrong. But I'm inclined to think that `MaterializeTemporaryExpr` should mirror the behavior of a `VarDecl`: the array bound should be filled in in the AST representation based on the initializer.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88236/new/

https://reviews.llvm.org/D88236