[PATCH] D59692: [ASTImporter] Fix name conflict handling with different strategies

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik accepted this revision.
shafik added a comment.

Other than my two comment this LGTM



================
Comment at: clang/lib/AST/ASTImporter.cpp:3452
         << FoundField->getType();
-
-      return make_error<ImportError>(ImportError::NameConflict);
+      ConflictingDecls.push_back(FoundField);
     }
----------------
I am a little concerned about this change. I am wondering if this was previously handled differently as a band-aid for some other issues that only showed up in rare cases when they iterated over all the results but not when they errored out on the first. 

Could we make the changes to `VisitFieldDecl` a separate PR so it is easier to revert later on if we do find this causes a regression we are not currently covering?


================
Comment at: clang/unittests/AST/ASTImporterTest.cpp:5615
+
+TEST_P(ConflictingDeclsWithLiberalStrategy, Typedef) {
+  Decl *ToTU = getToTuDecl(
----------------
Since the "Liberal" strategy should be the current status quo, I think it might make sense to have a first PR that just has the `*LiberalStrategy` tests to verify that indeed this is the current behavior as we expect.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59692