[PATCH] D53655: [ASTImporter] Fix redecl chain of classes and class templates
Gabor Marton via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Nov 27 07:43:53 PST 2018
martong marked 20 inline comments as done.
martong added inline comments.
================
Comment at: include/clang/ASTMatchers/ASTMatchers.h:1169
+/// matches 'a'.
+extern const internal::VariadicDynCastAllOfMatcher<Decl, IndirectFieldDecl>
+ indirectFieldDecl;
----------------
aaron.ballman wrote:
> Be sure to update Registry.cpp and regenerate the AST matcher documentation by running clang\docs\tools\dump_ast_matchers.py.
>
> This change feels orthogonal to the rest of the patch; perhaps it should be split out into its own patch?
> This change feels orthogonal to the rest of the patch; perhaps it should be split out into its own patch?
I agree this part could go into a separate patch, but the first use of this new ASTMatcher is in the new unittests of this patch, so I think it fits better to add them here.
================
Comment at: lib/AST/ASTImporter.cpp:2644
- PrevDecl = FoundRecord;
-
- if (RecordDecl *FoundDef = FoundRecord->getDefinition()) {
- if ((SearchName && !D->isCompleteDefinition() && !IsFriendTemplate)
- || (D->isCompleteDefinition() &&
- D->isAnonymousStructOrUnion()
- == FoundDef->isAnonymousStructOrUnion())) {
- // The record types structurally match, or the "from" translation
- // unit only had a forward declaration anyway; call it the same
- // function.
+ if (IsStructuralMatch(D, FoundRecord)) {
+ RecordDecl *FoundDef = FoundRecord->getDefinition();
----------------
a_sidorin wrote:
> IsStructuralMatch check will be repeated if (!SearchName && IsStructuralMatch). Is it expected behaviour?
Yes, this is intentional.
The first check does not emit any diagnostics. However, in the case you mention (`if (!SearchName && IsStructuralMatch)`) we have to emit diagnostics, because we found a real mismatch.
================
Comment at: lib/AST/ASTImporter.cpp:2667
ConflictingDecls.push_back(FoundDecl);
- }
+ } // for
----------------
a_sidorin wrote:
> Szelethus wrote:
> > Hah. We should do this more often.
> Unfortunately, it is a clear sign that we have to simplify the function. It's better to leave a FIXME instead.
I agree. Further, we should simplify all `Visit*Decl` functions where we iterate over the lookup results. The whole iteration could be part of a subroutine with a name like `FindEquivalentPreviousDecl`. But, I'd do that as a separate patch which focuses only on that refactor.
================
Comment at: lib/AST/ASTImporter.cpp:5064
+ if (!ToTemplated->getPreviousDecl()) {
+ auto *PrevTemplated =
+ FoundByLookup->getTemplatedDecl()->getMostRecentDecl();
----------------
aaron.ballman wrote:
> Do not use `auto` here as the type is not spelled out in the initialization.
Good catch.
================
Comment at: unittests/AST/ASTImporterTest.cpp:3794
+
+TEST_P(ImportFriendClasses, DeclsFromFriendsShouldBeInRedeclChains2) {
+ Decl *From, *To;
----------------
a_sidorin wrote:
> Will `DeclsFromFriendsShouldBeInRedeclChains` without number appear in another patch?
Sorry, `2` is an obsolete postfix. Removed it.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D53655/new/
https://reviews.llvm.org/D53655
More information about the cfe-commits
mailing list