ayzhao 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();
----------------
rsmith wrote:
> 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.
Clang simply does not allow braced-initialization for `VariableArrayType`, even
if `n` is greater than or equal to the number of initializers given:
https://godbolt.org/z/MaPjvn694. I updated the patch to make parenthesized
aggregate initialization do the same thing.
================
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);
----------------
rsmith wrote:
> 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?
> 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).
Done
> Also, do we ever need to preserve type qualifiers from the original entity's
> type?
I don't think so. In the case of `IncompleteArrayType`, which is the only
situation where we would need to deduce the `ConstantArrayType` based on the
number of arguments, the corresponding `QualType` object is always created with
`Quals = 0` [0].
[0]:
https://github.com/llvm/llvm-project/blob/c95533a7be2858893ec32b8abaa37a2d912ebe63/clang/lib/AST/ASTContext.cpp#L3888
================
Comment at: clang/lib/Sema/SemaInit.cpp:5401
+ InitializedEntity SubEntity =
+ InitializedEntity::InitializeBase(S.getASTContext(), &Base, false);
+ if (EntityIndexToProcess < Args.size()) {
----------------
rsmith wrote:
> Does this have the same wrong-lifetime-kind problem as members did?
I don't think so:
1. This will never initialize a reference because it's initializing a base
class.
1. In the case where the constructor of a base class accepts a reference
parameter, the lifetime is not extended: https://godbolt.org/z/MaqhTr1rP
================
Comment at: clang/lib/Sema/SemaInit.cpp:5476
+ InitializedEntity SubEntity =
+ InitializedEntity::InitializeMemberFromDefaultMemberInitializer(
+ FD);
----------------
rsmith wrote:
> 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.)
This now uses `InitializeMemberFromParenAggInit(..)` (before, it would emit
[the same error it emits for braced init
lists](https://godbolt.org/z/d3qfzh73z)). I also added a test for this.
================
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(
----------------
rsmith wrote:
> 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.
GCC rejects value initialization of references: https://godbolt.org/z/nhhehrf7r
This patch currently rejects value initialization of references, but I updated
the diagnostics to match those Clang emits with braced initialization:
https://godbolt.org/z/rbh17ecqe
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D148274/new/
https://reviews.llvm.org/D148274
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
