This is an archive of the discontinued LLVM Phabricator instance.

Preserve CXX method overrides in ASTImporter
ClosedPublic

Authored by lhames on Jun 19 2017, 3:34 PM.

Details

Summary

The ASTImporter should import CXX method overrides from the source context
when it imports a method decl.

Event Timeline

lhames created this revision.Jun 19 2017, 3:34 PM
lhames set the repository for this revision to rL LLVM.

To allow this to be tested I've added a flag to clang-import-test to dump the AST for the expression module, and modified clang's AST dumper code to print the list of overrides for each CXXMethodDecl. This should allow us to verify that override lists are preserved by AST imports.

The new dump format for CXXMethodDecl looks like:

| |-CXXMethodDecl 0x7fa9de878950 <line:6:3, col:24> col:8 used foo 'void (void)'
| | |-Overrides: [ 0x7fa9de89ce70 Base::foo 'void (void)' ]
| | |-CompoundStmt 0x7fa9de89ce38 <col:23, col:24>
| | `-OverrideAttr 0x7fa9de8789e8 <test.cpp:1:87>

It currently prints the full override list. Duncan and I had talked about printing only the "most recent" override, but that gets tricky where multiple inheritance is involved so I've opted for the simple scheme for now.

spyffe requested changes to this revision.Jun 20 2017, 10:51 AM

Thanks for working on this! I have a few requests (some may arise from ignorance of capture semantics).

lib/AST/ASTDumper.cpp
1190

It looks like you only need OS for this. Would it make sense to explicitly capture that here?

1197

Is by-value the appropriate way to capture dumpOverride here? It seems like extra work.

lib/AST/ASTImporter.cpp
5510

This looks weird to me given that the only caller has ToMethod handy.
Rather than force another lookup, I'd just pass ToMethod in.

tools/clang-import-test/clang-import-test.cpp
257

I slightly prefer passing in DumpAST and letting the top level logic decide when to set it to true. I don't like setting a precedent of having Parse know whether it's handling the top-level expression or one of the dependencies, because it can make it harder to add features like indirection later.

327

Instead of true, pass DumpAST here (see above comment).

This revision now requires changes to proceed.Jun 20 2017, 10:51 AM
lhames added inline comments.Jun 20 2017, 1:27 PM
lib/AST/ASTDumper.cpp
1190

I stuck to the convention used in nearby code, but we could just use explicit capture of 'OS' or 'this'.

1197

dumpOverride only captures 'this' so it makes no practical difference whether we catch it by value or reference. For a larger lambda we'd certainly want to capture by reference.

lib/AST/ASTImporter.cpp
5510

Sure.

tools/clang-import-test/clang-import-test.cpp
257

Good call - will update this.

lhames updated this revision to Diff 103264.Jun 20 2017, 1:50 PM
lhames edited edge metadata.

Updating based on Sean's comments.

spyffe accepted this revision.Jun 20 2017, 1:53 PM

Looks good to me. Upon reflection, I think it's more important to adhere to the standard of the surrounding code in the ASTDumper, otherwise you're hurting reusability.

This revision is now accepted and ready to land.Jun 20 2017, 1:53 PM
lhames closed this revision.Jun 20 2017, 2:06 PM

Committed in 305850, with a fix to handle destructor printing (which broke the Sema/ms_class_layout.cpp test case) in 305860.