ilya-biryukov added a comment.
Thanks!
I have two major comments and also inline NITs. Not sure if we should block on
those, just wanted to hear your opinions:
- `InitListExpr` and `CXXParenInitListExpr` have some code in common. Code
duplication is substantial and I think sharing the common implementation is
warranted. E.g. if some code would want to change something with `ArrayFiller`
or make a use of it, the work will probably need to be duplicated. To reiterate
what I mentioned earlier, I believe deriving `CXXParenInitListExpr` from
`InitListExpr` is not a very good option, but we might explore other
possibilities: a base class or factoring out common code in a separate struct.
I believe this could be done with a follow-up change, though, as the amount of
changes required does not seem too big, I would be happy with first landing
this patch and then cleaning up with a follow-up change.
- Did we explore the possibility of modelling this differently and somehow
avoiding making `CXXParenInitListExpr` an expression. This seems to borrow from
`InitListExpr`, but maybe for the wrong reasons. Braced initializers can
actually occur in expressions positions, e.g. `int a = {123}`. However,
`CXXParenInistListExpr` cannot occur in expression positions outside
initializations, e.g. having `CXXFunctionalCastExpr` for
`aggregate_struct(123)` feels somewhat wrong to me and seems to lead to
confusing error messages too. Maybe we should model it as a new expression that
contains both the type and the argument list? I.e. more similar to
`CXXConstructExpr`?
================
Comment at: clang/lib/Sema/SemaInit.cpp:5455
+ // All of the initialized entities are explicitly initialized by
+ // expressionse in the CXXParenListInitExpr. The semantic form is the
+ // same as the syntatic form
----------------
NIT: s/expressionse/expressions
================
Comment at: clang/lib/Sema/SemaInit.cpp:9803
+ TryOrBuildParenListInitialization(S, Entity, Kind, Args, *this,
+ /*VerifyOnly=*/false);
}
----------------
NIT: add break
================
Comment at: clang/lib/Sema/TreeTransform.h:14040
+ ArrayRef<Expr *> InitExprs = E->getInitExprs();
+ if (TransformExprs(reinterpret_cast<Expr *const *>(InitExprs.data()),
+ InitExprs.size(), true, TransformedInits))
----------------
`reinterpret_cast` seems redundant here or am I missing something?
================
Comment at: clang/lib/Serialization/ASTReaderStmt.cpp:2172
+ VisitExpr(E);
+ unsigned expectedNumExprs = Record.readInt();
+ assert(E->NumExprs == expectedNumExprs &&
----------------
NIT: `ExpectedNumExprs`
================
Comment at: clang/lib/Serialization/ASTReaderStmt.cpp:2181
+ E->getTrailingObjects<Expr *>()[I] = Record.readSubExpr();
+ E->updateDependence();
+
----------------
NIT: maybe add a comment that dependence does not depend on filler or syntactic
form. This might be non-obvious for someone who did not look into the
implementation.
Alternatively, you could run this at the end of this function, it should not be
confusing to anyone that certain properties get computed *after* the expression
was fully deserialized.
================
Comment at: clang/test/CXX/class/class.compare/class.spaceship/p1.cpp:106
Cmp<G2>() <=> Cmp<G2>(), // expected-note-re {{in defaulted three-way
comparison operator for '{{.*}}Cmp<{{.*}}G2>' first required here}}j
- // expected-error@#cmp {{no matching conversion for static_cast from
'void' to 'std::strong_ordering'}}
+ // expected-error@#cmp {{static_cast from 'void' to
'std::strong_ordering' is not allowed}}
Cmp<H>() <=> Cmp<H>(), // expected-note-re {{in defaulted three-way
comparison operator for '{{.*}}Cmp<{{.*}}H>' first required here}}j
----------------
Do you have any idea why did this diagnostic change?
================
Comment at: clang/test/CXX/drs/dr2xx.cpp:161
+ void test6(A::S as) { struct f {}; (void) f(as); } // pre20-error {{no
matching conversion}} pre20-note +{{}}
+ // post20-error@-1 {{functional-style cast from 'A::S' to 'f' is not
allowed}}
};
----------------
This might be related to other diagnostic change.
Why do we see the new behavior here? Do you think we should keep the old
behavior maybe?
I feel that both errors are communicating the same thing and probably keeping
the current state is acceptable, just want to better understand how we got
there.
It definitely has something to do with the initialization code, but I struggle
to understand what exactly changed
================
Comment at: clang/test/CXX/temp/temp.decls/temp.variadic/p4.cpp:131
+// pre20-error@-2 {{no matching constructor for initialization of 'B'}}
+// post20-error@-3 {{excess elements in struct initializer}}
+// post20-error@-4 {{excess elements in struct initializer}}
----------------
Given how pervasive this error is right now, I feel that we want to add a name
of the struct to this message.
This case is also a good example of how this diagnostic can be really low
quality with templates: it's unclear which exact base class causes a problem
here from the compiler output,.
Maybe open a GH issue for that? It seems like an independent task that will
also affect braced initializers and may need test file updates.
================
Comment at: clang/test/PCH/cxx_paren_init.cpp:30
+// CHECK: ret void
\ No newline at end of file
----------------
NIT: add a newline
================
Comment at: clang/test/PCH/cxx_paren_init.h:6
+void bar(int i, int j) { int arr[4](i, j); };
\ No newline at end of file
----------------
NIT: add a newline
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D129531/new/
https://reviews.llvm.org/D129531
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
