Page MenuHomePhabricator

[ASTImporter] Do not try to remove invisible Decls from DeclContext
ClosedPublic

Authored by martong on May 14 2018, 9:38 AM.

Details

Summary

DeclContext is essentially a list of Decls and a lookup table (LookupPtr) but these are encapsulated.
E.g. LookupPtr is private. DeclContext::removeDecl has the responsibility to remove the given decl from the list and from the table too.
Template specializations cannot participate in normal (qualified) lookup.
Consequently no template specialization can be in the lookup table, but they will be in the list. When the lookup table is built or when a new Decl is added, then it is checked whether it is a template specialization and if so it is skipped from the lookup table.
Thus, whenever we want to remove a Decl from a DeclContext we must not reach the point to remove that from the lookup table (and that is what this patch do).

With respect to ASTImporter: At some point probably we will be reordering FriendDecls and possibly CXXMethodDecls in a RecordDecl at importDeclContext to be able to structurally compare two RecordDecls.
When that happens we most probably want to remove all Decls from a RecordDecl and then we would add them in the proper order.

Diff Detail

Event Timeline

martong created this revision.May 14 2018, 9:38 AM

Hi Gabor,

I don't feel I'm a right person to review AST-related part so I'm adding other reviewers.
What I'm worrying about is that there is no test to check if our changes in removeDecl are correct. Maybe D44100 is a right patch for this but we should land it first or set dependencies properly.
Regarding ASTImporter, you can find my comments inline.

lib/AST/DeclBase.cpp
1410

specializations

unittests/AST/ASTImporterTest.cpp
1800

These tests seem to be for ImportDecl, not for ImportExpr.

1833

Check for namespaceDecl() looks too weak because import of NamespaceDecl succeeds even if import of its nested decls fails. Same below.

1857

For this change, we should create a separate patch.

martong updated this revision to Diff 146845.May 15 2018, 8:43 AM
martong marked 4 inline comments as done.
  • Add test for removeDecl, remove unrelated tests

Hi Aleksei,

Thanks for reviewing this.
I could synthesize a test which exercises only the DeclContext::removeDecl function. This test causes an assertion without the fix.
Removed the rest of the testcases, which are not strictly connected to this change.

unittests/AST/ASTImporterTest.cpp
1857

This test is disabled ATM, but I agree it would be better to bring this in when we fix the import of friends.

martong updated this revision to Diff 146847.May 15 2018, 8:47 AM
  • Remove unrelated CXX14 changes

Can you roll back the changes in MatchVerifier.h? Phab is telling me that the only changes are to whitespace.

lib/AST/DeclBase.cpp
1353

Rather than make a hanging forward declaration, can you just move the definition of this function here?

1411

The return should be on its own line per our usual conventions.

martong updated this revision to Diff 147274.May 17 2018, 2:13 AM

Addressing review comments.

martong marked 2 inline comments as done.May 17 2018, 2:14 AM
This revision is now accepted and ready to land.May 17 2018, 5:39 AM
martong closed this revision.May 18 2018, 2:23 AM

Closed by commit: rL332699