[PATCH] D140587: [clang] Fix a clang crash on invalid code in C++20 mode.

[Ilya Biryukov via Phabricator via cfe-commits]

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