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