This is an archive of the discontinued LLVM Phabricator instance.

[ASTImporter] Fix redecl chain of classes and class templates
ClosedPublic

Authored by martong on Oct 24 2018, 9:38 AM.

Details

Summary

The crux of the issue that is being fixed is that lookup could not find
previous decls of a friend class. The solution involves making the
friend declarations visible in their decl context (i.e. adding them to
the lookup table).
Also, we simplify VisitRecordDecl greatly.

This fix involves two other repairs (without these the unittests fail):
(1) We could not handle the addition of injected class types properly
when a redecl chain was involved, now this is fixed.
(2) DeclContext::removeDecl failed if the lookup table in Vector form
did not contain the to be removed element. This caused troubles in
ASTImporter::ImportDeclContext. This is also fixed.

Diff Detail

Event Timeline

martong created this revision.Oct 24 2018, 9:38 AM

I can't add anything meaningful to the conversation, but I spotted some nits, so here they are.

include/clang/ASTMatchers/ASTMatchers.h
1167

Did you mean to write indirectFieldDecl()?

lib/AST/ASTImporter.cpp
2680

Hah. We should do this more often.

lib/AST/DeclBase.cpp
1469–1471

Contained in?

balazske added inline comments.Oct 26 2018, 2:38 AM
lib/AST/ASTImporter.cpp
5070

This is a long line that should be split.

martong updated this revision to Diff 174893.Nov 21 2018, 3:24 AM
martong marked 4 inline comments as done.
  • Minor style changes and rename a function
martong added inline comments.Nov 21 2018, 3:24 AM
lib/AST/DeclBase.cpp
1469–1471

Indeed, containedInVector sounds better, so I renamed.

balazske added inline comments.Nov 22 2018, 1:51 AM
lib/AST/DeclBase.cpp
1469–1471

For me, containsInVector is the better name, or hasInVector ("contains" is already used at other places but not "contained" and it sounds like it is not contained any more).

Szelethus added inline comments.Nov 22 2018, 7:10 AM
lib/AST/DeclBase.cpp
1469–1471

Sorry about the confusion, my inline was only about the comment above it, that it isn't obvious enough that what decl is contained in. But after taking a second look, it's very clear that only Map is mutated in this context, so I don't insist on any modification :)

martong updated this revision to Diff 175058.Nov 22 2018, 8:51 AM
  • Revert "Minor style changes and rename a function"
martong marked 3 inline comments as done.Nov 22 2018, 8:51 AM
martong added inline comments.
lib/AST/DeclBase.cpp
1469–1471

Okay, so I just reverted the name change.

I see that now everything is reverted, the "good" things too (change to indirectFieldDecl and a line split)?

martong updated this revision to Diff 175091.Nov 23 2018, 1:44 AM
martong marked an inline comment as done.
  • Keep the good changes and use the name 'containsInVector'

I see that now everything is reverted, the "good" things too (change to indirectFieldDecl and a line split)?

Yes, you are completely right, sorry for this mess. I just have updated so the good things remain.

balazske accepted this revision.Nov 23 2018, 5:32 AM

Do not forget that there is a fix the to use getMostRecentDecl in ASTImporter.cpp line 2666 here.

This revision is now accepted and ready to land.Nov 23 2018, 5:32 AM
martong updated this revision to Diff 175127.Nov 23 2018, 7:06 AM
  • Use MostRecentDecl when setting PrevDecl

This will break LLDB, unless https://reviews.llvm.org/D54863 is applied. @shafik Could you please take a look on https://reviews.llvm.org/D54863 ?

Hi Gabor,

The change looks mostly Ok but there are some comments inline.

include/clang/AST/DeclContextInternals.h
128

auto I = llvm::find(Vec, D)?

lib/AST/ASTImporter.cpp
2646

because (typo)

2657–2659

IsStructuralMatch check will be repeated if (!SearchName && IsStructuralMatch). Is it expected behaviour?

2680

Unfortunately, it is a clear sign that we have to simplify the function. It's better to leave a FIXME instead.

2834

Are these lines removed due to move of MapImported into Create helper?

5007–5009

Space after '*'?

unittests/AST/ASTImporterTest.cpp
3794

Will DeclsFromFriendsShouldBeInRedeclChains without number appear in another patch?

Please note that changes in AST lib will require AST code owners' approval. @rsmith, @aaron.ballman could you please take a look?

aaron.ballman added inline comments.Nov 25 2018, 12:54 PM
include/clang/AST/DeclContextInternals.h
125

const NamedDecl *D

126

Please add a string literal to the assert explaining what is being checked here.

128

I'd go with return llvm::is_contained(Vec, D);

include/clang/ASTMatchers/ASTMatchers.h
1169

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?

lib/AST/ASTImporter.cpp
2737

Drop the top-level const qualifier.

2738

Elide braces; I'd probably sink the IsFriend into here since it's not used elsewhere.

5007–5009

Space before the asterisk. ;-)

5070

Do not use auto here as the type is not spelled out in the initialization.

5079

Elide braces.

martong marked 20 inline comments as done.Nov 27 2018, 7:43 AM
martong added inline comments.
include/clang/ASTMatchers/ASTMatchers.h
1169

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.

lib/AST/ASTImporter.cpp
2657–2659

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.

2680

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.

5070

Good catch.

unittests/AST/ASTImporterTest.cpp
3794

Sorry, 2 is an obsolete postfix. Removed it.

martong updated this revision to Diff 175482.Nov 27 2018, 7:43 AM
martong marked 5 inline comments as done.
  • Address several minor review comments
aaron.ballman accepted this revision.Nov 27 2018, 1:56 PM

This seems mostly reasonable to me, but @rsmith is more well-versed in this area and may have further comments. You should give him a few days to chime in before committing.

include/clang/AST/DeclContextInternals.h
125–129

Given that this is only called in one place and is effectively a one-liner, I'd get rid of this function entirely and replace the call below.

include/clang/ASTMatchers/ASTMatchers.h
1169

Okay, that's fair enough.

martong marked 4 inline comments as done.Nov 28 2018, 2:09 AM
martong added inline comments.
include/clang/AST/DeclContextInternals.h
125–129

Good catch, thank you.

lib/AST/ASTImporter.cpp
2834

Yes, exactly.

martong updated this revision to Diff 175641.Nov 28 2018, 2:10 AM
martong marked 2 inline comments as done.
  • Remove containsInVector

@aaron.ballman Thanks for your review. I have updated the patch to remove containsInVector.

This seems mostly reasonable to me, but @rsmith is more well-versed in this area and may have further comments. You should give him a few days to chime in before committing.

Of course.

@rsmith Please raise any objections until Dec 14 (or if this deadline is too strict). I'd like to commit this next week latest so it can get in still this year.

This revision was automatically updated to reflect the committed changes.