Page MenuHomePhabricator

Preserve CXX method overrides in ASTImporter

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



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).


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


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


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


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.


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

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


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.




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.