visitor pattern: template vs virtual

[Ellers]

Hi all,

I'm doing a draft visitor pattern implementation for TinyXML.

I've got a draft template version going first, and it works well.
You can invoke it with any class you like that implements certain methods.
For example:

// load a doc
TiXmlDocument doc("demo.xml" );
bool ok= doc.LoadFile();
   
// write document structure to stdout, just to see what nodes are where:
visit(&doc, CTiXmlDumpStream(stdout));

// Write document as XML to STDOUT
// (this one can write to files too)
CTiXmlFILEWriter writer(stdout, TIX_INDENT_AUTO|TIX_NEWLINE_AUTO);
visit(&doc, writer)

// Write document to a string
CTiXmlBufferWriter writer2(4096);
writer2.options = TIX_INDENT_AUTO|TIX_NEWLINE_AUTO;
visit(&doc, writer2);
printf( "XML in string form:\n%s", writer2.buffer);

I like this because
(a) lots of different visitors/writers can be written relatively easily;
(b) none of the original classes needed to be changed;
(c) the original API like Print() can be left in, and replaced with

TiXmlBase::Print(name)
{
   f=fopen(name);
   CTiXmlFILEWriter writer(stdout, TIX_INDENT_AUTO|TIX_NEWLINE_AUTO);
   visit(this, writer)
   fclose(f);
}

(d) it gives lots of flexibility for output options (newline, indent, BOM)
without it needing to be "baked in" to the TiXml* classes.

All of this works well so far. This is current visit() function:

template <class VisitorT>
void visit( TiXmlNode* pParent, VisitorT& visitor)
{
    if ( !pParent ) return;

    visitor.OnBeginVisit();
    visit(pParent,visitor,0);
    visitor.OnEndVisit();
}

This calls down to the following switch-based visitor implementation
(details omitted for readability):

template <class VisitorT>
void visit( TiXmlNode* pParent, VisitorT& visitor, unsigned int indent )
{
    switch ( t )
    {
    case TiXmlNode::DOCUMENT:
        visitor.OnDocument(pParent->ToDocument());
        break;

    case TiXmlNode::ELEMENT:
        visitor.OnElement(pElem, pAttrib!=0, pChild!=0);
        // then visit all attributes
        //...
        break;

    case TiXmlNode::COMMENT:
        visitor.OnComment(pParent->ToComment());
        break;

    case TiXmlNode::UNKNOWN:
        visitor.OnUnknown(pParent->ToUnknown());
        break;

    case TiXmlNode::TEXT:
        visitor.OnText(pParent->ToText());
        break;

    case TiXmlNode::DECLARATION:
        visitor.OnDeclaration(pParent->ToDeclaration());
        break;

    default:
        break;
    }
}

Now, this is ok in that it works, and its fairly simple to follow.
But its not a "real" visitor implementation.
What I mean is, its got a switch statement in there that could be avoided by
using a virtual function like:

class TiXmlElement ...
{

    /** Visitor Implementation.
    It would be nice to use templates here but I'm not sure if I can do
    virtual template, and then I'd be worried not all compilers will support it.
    */
    virtual void visit(TiXmlVisitor* visitor)
    {
        visitor->OnElement(this);
        // visit attributes
    }
};

I'm bipolar on these topics. On the one hand, the first version works,
hasn't needed any changes to the API of the other classes, and is simple.

On the other, the switch-based approach isn't very nice, so a virtual method
approach could be used, e.g.:

class TiXmlVisitor
{
  virtual void OnElement( TiXmlElement* );
  ...
};

Pros of virtual approach:
- virtual table instead of switch (FWIW)
- widely known
- portable/safe subset of C++ (ie no templates required)

Cons:
- requires that a base interface be defined
  (template approach doesn't need that)
- modification of API (but additions only)
- arguably less elegant (but very subjective)

(I'm not sure if a "virtual template" method approach works).

Any comments/preferences?
Thanks
Ellers

[Lee Thomason]

I think the virtual approach, without a template, is stronger. In this version, we would define (names subject to change) an interface:

class TiXmlVisitInterface {
  public:
    virtual void VisitThis( const TiXmlNode* node, Whatever... )
}

The user class that wants to be *visited* would inherit from TiXmlVisitInterface, and then call:

doc.Visit( this );

And the recursive walk would then call all the VisitThis methods. (Bad name, again. :) ) I think that is what you were suggesting in the last approach, but wanted to be sure.

Concern on the template approach:
I don't like introducing templates. TinyXML has a good thing going with working on 'limited' systems, and templates are begging for trouble in those environments. A pure abstract class should compile anywhere, and if you have some kooky compiler that doesn't like MI, then you just don't use the Visit() functionality, and all is well.

Pros of the visit thing:
Nice way to solve a range of problem: formatted output, string building, etc. This is a good thing.

Using the virtual class callback is a little advanced, but I think the whole "tree walk" is a little advanced, so that seems consistent.

Cons:
Changing the tree in the visit would cause TinyXML to crash. In addition to documenting this, I think all the visit methods and objects they pass to the client should be 'const' to discourage change during the walk.

thougts?
lee

[Ellers]

Cool. Yep, we're definitely on the same page.

As much as I like templates in my own stuff, I don't think there's really a need in this case; interface/inheritance will do the job.

I think as a general rule modifying a document structure while iterating the document is considered bad form, and results in "unpredictable results". So using const forms would be fine, and banning doc modification during a visit seems reasonable to me, at least for now, and probably forever.

I'll post when I've got a non-templated version demonstrable...

Thanks for the feedback :)

Ellers