This is an archive of the discontinued LLVM Phabricator instance.

CodeGen: Clean up implementation of vtable initializer builder. NFC.
ClosedPublic

Authored by pcc on Jul 21 2016, 12:30 PM.

Details

Summary
  • Simplify signature of CreateVTableInitializer function.
  • Move vtable component builder to a separate function.
  • Remove unnecessary accessors from VTableLayout class.

This is in preparation for a future change that will alter the type of the
vtable initializer.

Diff Detail

Repository
rL LLVM

Event Timeline

pcc updated this revision to Diff 64940.Jul 21 2016, 12:30 PM
pcc retitled this revision from to CodeGen: Clean up implementation of vtable initializer builder. NFC..
pcc updated this object.
pcc added a reviewer: rsmith.
pcc added a subscriber: cfe-commits.
pcc updated this revision to Diff 70622.Sep 7 2016, 4:45 PM
  • Refresh
rsmith accepted this revision.Sep 7 2016, 5:00 PM
rsmith edited edge metadata.
rsmith added inline comments.
include/clang/AST/VTableBuilder.h
222–225 ↗(On Diff #70622)

Can you remove these? It looks like they should be unused now.

lib/CodeGen/CGVTables.cpp
527–528 ↗(On Diff #70622)

This is just CGM.PtrDiffTy.

588 ↗(On Diff #70622)

Do you have any idea why we're doing this? It looks wrong to me. These ABI entry points are exposed and could certainly have their addresses taken and used in this translation unit.

598–615 ↗(On Diff #70622)

Please remove the else-after-return throughout here. That'll make it much more obvious to a reader of the code that fallthrough to the next case is not possible.

lib/CodeGen/CGVTables.h
52 ↗(On Diff #70622)

I'd prefer these to be two separate declarations, with doc comments.

This revision is now accepted and ready to land.Sep 7 2016, 5:00 PM
pcc marked 4 inline comments as done.Sep 7 2016, 6:23 PM
pcc added inline comments.
lib/CodeGen/CGVTables.cpp
588 ↗(On Diff #70622)

I introduced this in D18071. Although a translation unit can take the address of one of these functions, that would involve declaring a function with a reserved name, so I believe we'd be allowed to impose restrictions such as unnamed_addr on the address.

This revision was automatically updated to reflect the committed changes.