[PATCH] D148274: [clang] Fix overly aggressive lifetime checks for parenthesized aggregate initialization

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

rsmith added inline comments.


================
Comment at: clang/lib/Sema/SemaInit.cpp:5364-5368
     if (const ConstantArrayType *CAT =
             S.getASTContext().getAsConstantArrayType(Entity.getType()))
       ArrayLength = CAT->getSize().getZExtValue();
     else
       ArrayLength = Args.size();
----------------
What happens if the array is of `VariableArrayType` or `DependentSizedArrayType`? I guess we shouldn't get here in the latter case, but the former case seems possible, and presumably shouldn't result in constructing a value of `ConstantArrayType`. [Test](https://godbolt.org/z/377TWzn7r):

```
constexpr int f(int n, int i) {
    int arr[n](1, 2, 3);
    return arr[i];
}

constexpr int a = f(1, 2);
constexpr int b = f(4, 3);
```

GCC appears to leave the type alone in this case, and treats the evaluation as UB if `n` is less than the number of initializers given. That matches what GCC does for a `{...}` initializer of a VLA. We should probably match what Clang does for `{...}` initialization of a VLA and reject.


================
Comment at: clang/lib/Sema/SemaInit.cpp:5391-5393
+    ResultType = S.Context.getConstantArrayType(
+        AT->getElementType(), llvm::APInt(/*numBits=*/32, ArrayLength),
+        /*SizeExpr=*/nullptr, ArrayType::Normal, 0);
----------------
It would be nice to use the original type here in the case where we didn't add an array bound, so we preserve type sugar (typedefs etc). Also, do we ever need to preserve type qualifiers from the original entity's type?


================
Comment at: clang/lib/Sema/SemaInit.cpp:5401
+        InitializedEntity SubEntity =
+            InitializedEntity::InitializeBase(S.getASTContext(), &Base, false);
+        if (EntityIndexToProcess < Args.size()) {
----------------
Does this have the same wrong-lifetime-kind problem as members did?


================
Comment at: clang/lib/Sema/SemaInit.cpp:5476
+          InitializedEntity SubEntity =
+              InitializedEntity::InitializeMemberFromDefaultMemberInitializer(
+                  FD);
----------------
Does this entity kind do the right thing for lifetime warnings? (I'm not sure why this is a distinct kind of `InitializedEntity`; the thing that changes here is not the entity, it's how it's initialized.)


================
Comment at: clang/lib/Sema/SemaInit.cpp:5486-5487
+          //   The remaining elements...otherwise are value initialzed
+          InitializedEntity SubEntity =
+              InitializedEntity::InitializeMember(FD, &Entity);
+          InitializationKind SubKind = InitializationKind::CreateValue(
----------------
Is there any possibility of lifetime warnings here? I don't *think* value initialization can ever create problems, but it would seem more obviously right to use the parenthesized aggregate initialization entity kind here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148274