[PATCH] D129640: [clang][ASTImporter] Improved handling of functions with auto return type.

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Thanks, nice work!



================
Comment at: clang/lib/AST/ASTImporter.cpp:3237
+  ParentMapContext &ParentC = DC->getParentASTContext().getParentMapContext();
+  DynTypedNodeList P = ParentC.getParents(*S);
+  while (!P.empty()) {
----------------



================
Comment at: clang/lib/AST/ASTImporter.cpp:3237
+  ParentMapContext &ParentC = DC->getParentASTContext().getParentMapContext();
+  DynTypedNodeList P = ParentC.getParents(*S);
+  while (!P.empty()) {
----------------
martong wrote:
> 
The first call of `getParents` will create the parent map, via a full-blown AST visitation. I am concerned a bit about the additional performance overhead. Could you please run some measurements? (E.g. a CTU run on `protobuf` and `bitcoin` with our internal CI infra)


================
Comment at: clang/lib/AST/ASTImporter.cpp:3251-3254
+    if (Arg.getKind() == TemplateArgument::Type)
+      return hasTypeDeclaredInsideFunction(Arg.getAsType(), FD);
+    if (Arg.getKind() == TemplateArgument::Expression)
+      return isAncestorDeclContextOf(FD, Arg.getAsExpr());
----------------
Should this be handled in a `switch` rather, perhaps with an `llvm_unreachable` at the `default` case? Just to make sure that no "kind" is left out.


================
Comment at: clang/lib/AST/ASTImporter.cpp:3269-3273
+    // Note: It is possible that T can be get as both a RecordType and a
+    // TemplateSpecializationType.
+  }
+  if (const auto *TST = T->getAs<TemplateSpecializationType>()) {
+    return llvm::count_if(TST->template_arguments(), CheckTemplateArgument);
----------------
Is it possible that `T` is both a `RecordType` and a `TemplateSpecializationType` at the same time? From the [[ https://clang.llvm.org/doxygen/classclang_1_1TemplateSpecializationType.html | hierarchy ]] this seems impossible (?)


================
Comment at: clang/unittests/AST/ASTImporterTest.cpp:6323
 
+TEST_P(ImportAutoFunctions, ReturnWithTemplateWithIntegerArgDeclaredInside) {
+  Decl *FromTU = getTuDecl(
----------------
Nice test!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129640