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