a_sidorin added a comment.
Hi Gabor,
The change looks mostly Ok but there are some comments inline.
================
Comment at: include/clang/AST/DeclContextInternals.h:128
+ DeclsTy &Vec = *getAsVector();
+ DeclsTy::iterator I = std::find(Vec.begin(), Vec.end(), D);
+ return I != Vec.end();
----------------
`auto I = llvm::find(Vec, D)`?
================
Comment at: lib/AST/ASTImporter.cpp:2633
+ // struct/union and in case of anonymous structs. Would be false
+ // becasue there may be several anonymous/unnamed structs in a class.
+ // E.g. these are both valid:
----------------
because (typo)
================
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();
----------------
IsStructuralMatch check will be repeated if (!SearchName && IsStructuralMatch).
Is it expected behaviour?
================
Comment at: lib/AST/ASTImporter.cpp:2667
ConflictingDecls.push_back(FoundDecl);
- }
+ } // for
----------------
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.
================
Comment at: lib/AST/ASTImporter.cpp:2821
- Importer.MapImported(D, D2);
----------------
Are these lines removed due to move of MapImported into Create helper?
================
Comment at: lib/AST/ASTImporter.cpp:4991
if (IsStructuralMatch(D, FoundTemplate)) {
- if (!IsFriend) {
- Importer.MapImported(D->getTemplatedDecl(),
- FoundTemplate->getTemplatedDecl());
- return Importer.MapImported(D, FoundTemplate);
+ ClassTemplateDecl* TemplateWithDef = getDefinition(FoundTemplate);
+ if (D->isThisDeclarationADefinition() && TemplateWithDef) {
----------------
Space after '*'?
================
Comment at: unittests/AST/ASTImporterTest.cpp:3794
+
+TEST_P(ImportFriendClasses, DeclsFromFriendsShouldBeInRedeclChains2) {
+ Decl *From, *To;
----------------
Will `DeclsFromFriendsShouldBeInRedeclChains` without number appear in another
patch?
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
