[PATCH] D21410: [pdb] Change type visitor pattern to use dynamic polymorphism

[Reid Kleckner via llvm-commits]

rnk added a comment.

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.

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

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


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.

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


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


http://reviews.llvm.org/D21410