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

[Gabor Marton via Phabricator via cfe-commits]

martong marked 4 inline comments as done.
martong added inline comments.


================
Comment at: clang/lib/AST/ASTImporter.cpp:3452
         << FoundField->getType();
-
-      return make_error<ImportError>(ImportError::NameConflict);
+      ConflictingDecls.push_back(FoundField);
     }
----------------
shafik wrote:
> 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?
Ok, I reverted these hunks. I am going to create a follow-up patch which will include these changes.


================
Comment at: clang/unittests/AST/ASTImporterTest.cpp:5615
+
+TEST_P(ConflictingDeclsWithLiberalStrategy, Typedef) {
+  Decl *ToTU = getToTuDecl(
----------------
shafik wrote:
> 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.
Ok, I removed the test suite `ConflictingDeclsWithConservativeStrategy`. I am going to create a subsequent patch which adds comprehensive testing for both strategies.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59692