ilya-biryukov added subscribers: ayzhao, ilya-biryukov.
ilya-biryukov added inline comments.
================
Comment at: clang/lib/Sema/SemaInit.cpp:6177
// constructors. For example, conversion function.
if (const auto *RD =
dyn_cast<CXXRecordDecl>(DestType->getAs<RecordType>()->getDecl());
----------------
@ayzhao it's a bit surprising that this code path runs for braced
initializations even though the patch was written for parenthesized inits. Is
this intended?
================
Comment at: clang/lib/Sema/SemaInit.cpp:6179
dyn_cast<CXXRecordDecl>(DestType->getAs<RecordType>()->getDecl());
- S.getLangOpts().CPlusPlus20 && RD && RD->isAggregate() && Failed() &&
+ S.getLangOpts().CPlusPlus20 && RD && RD->hasDefinition() &&
+ RD->isAggregate() && Failed() &&
----------------
NIT: could you add a comment that we don't need to call `RequireCompleteType`
as `TryConstructorInitialization` already does this for us?
`RequireCompleteType` should be the default choice in general as we might need
to pick the right template specialization and do the corresponding template
instantiation. However, in this case it has already been done.
================
Comment at: clang/test/SemaCXX/recovery-expr-type.cpp:181
+A<int, int> foo() { // expected-error {{implicit instantiation of undefined
template}}
+ return A<int, int>{1}; // expected-error 2{{implicit instantiation of
undefined template}}
+}
----------------
Could you add a test for `A<int, int>(1)` too?
The code path somehow runs for `{}` too, but I think it's unintended and the
relevant code path might only run for parenthesized init.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D140587/new/
https://reviews.llvm.org/D140587
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
