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

[Zachary Turner via llvm-commits]

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.

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

On Wed, Jun 15, 2016 at 3:57 PM Reid Kleckner <rnk at google.com> wrote:

> 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
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160615/7993cbe8/attachment.html>