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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
