This is an archive of the discontinued LLVM Phabricator instance.

[pdb] Change type visitor pattern to use dynamic polymorphism
ClosedPublic

Authored by zturner on Jun 15 2016, 2:44 PM.

Details

Summary

This gets rid of the CRTP (curiously recurring template pattern) on the CTypeVisitor, and instead changes to a method of dynamic polymorphism to call back into the visitor implementation.

This has a number of advantages, including:

  1. Ability to implement CTypeVisitor in a .cpp file instead of a .h file
  2. Removal of a bunch of methods and code duplication since functionality of CTypeVisitorImpl can be merged with functionality of TypeDumper.
  3. It's possible to implement a simple visitor with MUCH less code than before. Since the base class provides virtual methods with default implementations, if all you want to do is handle a few simple method types, you need only override the few methods you care about. This can lead to much less code when writing a new visitation handler. Previously you would have to do the #define magic and implement every method in the class, and if you only wanted 2 or 3 methods to do something, there would be a lot of unnecessary function definitions.
  4. More robust error propagation. Since we don't rely on saving error state in a boolean anymore, we can simply propagate errors all the way out to the top level with actual llvm::Errors. This allows the implementations to propagate more information up the call stack.

Diff Detail

Repository
rL LLVM

Event Timeline

zturner updated this revision to Diff 60909.Jun 15 2016, 2:44 PM
zturner retitled this revision from to [pdb] Change type visitor pattern to use dynamic polymorphism.
zturner updated this object.
zturner added reviewers: rnk, ruiu.
zturner added a subscriber: llvm-commits.
ruiu accepted this revision.Jun 15 2016, 3:23 PM
ruiu edited edge metadata.

LGTM

lib/DebugInfo/CodeView/CVTypeVisitor.cpp
1 ↗(On Diff #60909)

Remove -*- C++ -*- since this file has .cpp extnesion.

21 ↗(On Diff #60909)

This is not directly related to this change, but should we write return {} instead of return Error::success() for the sake of brevity?

This revision is now accepted and ready to land.Jun 15 2016, 3:23 PM

I can't check the code at the moment but somewhere in Error.h it
specifically recommends using the explicit Error::success syntax

rnk edited edge metadata.Jun 15 2016, 3:57 PM

I guess on the whole I'm not in favor of this.

  1. Ability to implement CTypeVisitor in a .cpp file instead of a .h file

I don't see how this wasn't already possible. I could have merged CVTypeDumperImpl and CVTypeDumper if I wanted to, but I was trying to hide as many implementation details as possible and minimize the header.

  1. Removal of a bunch of methods and code duplication since functionality of CTypeVisitorImpl can be merged with functionality of TypeDumper.

CVTypeDumperImpl and CVTypeDumper don't have that much code duplication IMO. The other nice thing about the way this was structured is that clients of TypeDumper.h didn't depend on the visitor, and all of the methods of the type dumper were in an anonymous namespace, giving them internal linkage, reducing binary size, etc.

  1. It's possible to implement a simple visitor with MUCH less code than before. Since the base class provides virtual methods with default implementations, if all you want to do is handle a few simple method types, you need only override the few methods you care about. This can lead to much less code when writing a new visitation handler. Previously you would have to do the #define magic and implement every method in the class, and if you only wanted 2 or 3 methods to do something, there would be a lot of unnecessary function definitions.

This should have already been true. You don't have to use TypeRecord.def to declare every kind of visit method, you should be able to inherit the no-op visit##Kind method from the visitor.

  1. More robust error propagation. Since we don't rely on saving error state in a boolean anymore, we can simply propagate errors all the way out to the top level with actual llvm::Errors. This allows the implementations to propagate more information up the call stack.

This is great, but it only requires changing the return types of the visitor to Error.

I can't respond inline from mobile for some reason, but here's my comments:

  1. it wasn't possible before because CVTypeVisitor was a template class, so

it had to be implemented in the header. You could have merged them, yes,
but this would couple the visitation switch delegation with the handlers,
making it not reusable and defeating the whole purpose. This way, the
header is even more minimized than before, including the removal of many
#includes from the header, which is always a win.

I guess in general, I'm not in favor of templatizing anything unless the
advantages are very compelling (significantly less code, higher
performance, greater reusability, etc). If there's no practical difference,
i prefer dynamic polymorphism purely on readability and better compiler
errors.

  1. clients of TypeDumper.h still don't depend on the visitor, but they do

have access to the visitor should they want to use it. Internal linkage is
fine and has some advantages, but obviously hurts reusability (albeit by
design)

zturner updated this revision to Diff 60994.Jun 16 2016, 10:47 AM
zturner edited edge metadata.

Rebases on top of Rui's recent patches.

One added advantage of this approach is that, while rebasing, I actually caught a couple of bugs in Rui's patch that would not have been possible to catch at compile time otherwise. Mainly there were previously separate visitation functions for class, struct, and interface, but these are all implemented by a single record type, and thus have only a single visitation function. This was caught because when I added the override keyword to them, the compiler error'ed saying that there was no suitable function to override.

This revision was automatically updated to reflect the committed changes.
rnk added a comment.Jun 16 2016, 11:17 AM

One added advantage of this approach is that, while rebasing, I actually caught a couple of bugs in Rui's patch that would not have been possible to catch at compile time otherwise. Mainly there were previously separate visitation functions for class, struct, and interface, but these are all implemented by a single record type, and thus have only a single visitation function. This was caught because when I added the override keyword to them, the compiler error'ed saying that there was no suitable function to override.

Acking on the mailing list that I buy this reasoning, lgtm