copy contructor for TiXmlElement

[Anonymous]

a found that i really needed a copy ctor for TiXmlElements. (TiXmlElement::Clone() isn't always usable).So a hacked in the following:

TiXmlElement::TiXmlElement (const TiXmlElement& elm)

TiXmlNode( TiXmlNode::ELEMENT )
{
    firstChild = lastChild = 0;
    value = elm.value;

    elm.CopyToClone(this);   
        // Clone the attributes, then clone the children.

    TiXmlAttribute* attribute = 0;
    for(    attribute = elm.attributeSet.First();
    attribute;
    attribute = attribute->Next() )
    {
        SetAttribute( attribute->Name(), attribute->Value() );
    }

    TiXmlNode* node = 0;
    for ( node = elm.firstChild; node; node = node->NextSibling() )
    {
        LinkEndChild( node->Clone() );
    }

}

is this a good idea?

[Jens Niegemann]

Hi,
I needed an assignment operator or copy constructor for TiXmlElement as well and just used yours. I hope that's okay. It seems to work perfectly.
Could we add something like this (and an assignment operator) to the official code base?

bye...
...manilius

[Lee Thomason]

Good idea to have - I'll add it to features request.

lee

[John-Philip Leonard Johansson]

a little code design question about the above

make a
void TiXmlElement::Copy( const TiXmlElement& other )
{ /* put the above copy constructor code here instead */ }

Let the copy constructor call it, like so:
TiXmlElement::TiXmlElement (const TiXmlElement& other) : TiXmlNode( TiXmlNode::ELEMENT )
{ Copy( other ); }

and the same for operator =
TiXmlElement& TiXmlElement::operator = (const TiXmlElement& other)
{ Copy( other ); return *this; }

Then the Clone() function can be implemented like this:
TiXmlElement* Clone()
{ return new TiXmlElement(*this); }

What do you think? =)

[Anonymous]

yes, that would be the proper way to do things. As you probaly have seen i basicly copied the Clone() function into the copy contructor. And copy/pasting of code is generally a bad thing.

[Lee Thomason]

Does anyone have a patch that adds copy constructors and assignment operators? Or could send me source files so I could diff them in?

thanks,
lee

[John-Philip Leonard Johansson]

Didn't see the post until now, I'll happily code them and test them for you =)

If nobody already has done this I will start on tuesday.

[Lee Thomason]

Thanks!
lee

[John-Philip Leonard Johansson]

hi!

I would already be done by now, but the CVS is acting up. Can't view the online CVS at :
http://cvs.sourceforge.net/viewcvs.py/tinyxml
all I get is
"Bad Gateway
The proxy server received an invalid response from an upstream server."

And when I try a CVS checkout using Tortoise CVS, well I don't seem to get it to work =/ I'm using these settings:
CVSROOT: :pserver:anonymous@cvs.sf.net:/cvsroot/tinyxml
Protocol: Password server (:pserver)
Server: cvs.sf.net
Repository folder: /cvsroot/tinyxml
User name: anonymous
Module: tinyxml

Am I doing something wrong there? I'll get right on coding once I get down the latest source from the CVS =)

[Lee Thomason]

Are you still having this problem? If so, email me directly and I'll send you the current source files.

lee

[John-Philip Leonard Johansson]

seems like it was Sourceforge that had the problem? Well it works now and I've submitted a patch. Thank you kindly for the offer though.

[Lee Thomason]

Excellent! I'll look at it this weekend.

Thanks much,
lee

[Lee Thomason]

I didn't even get a chance to turn on my computer this weekend. Sorry about that. This coming weekend should be better! I'm very interested in the patch.

lee

[Lee Thomason]

Good patch, overall. Using the "CopyTo" method to target assignment, copy constructor, and clone makes a lot of sense.

I think the copy and assignment should be supported for all the TiXmlNodes, so that is taking a bit of time. Also (friendly reminder for everyone) the patch didn't include test cases, so I'm putting those in there too.

Work has begun! There will be copy constructors.

lee

[John-Philip Leonard Johansson]

Thank you =)

Sorry for not including test cases, and on a side note I think the test cases are to many and to clogged up in that main.cpp. But if I write another patch I'll put in a test case.

Yes I agree on TiXmlNodes. If you wish, I'll write them for you in the same manner I wrote them for TiXmlElement.

/JP

[Lee Thomason]

No problem - the test cases are very straightforward. They are also revealing some (very minor) issues with the code, so the debugging has been useful.

I have the copy/assignment written for all the sub-classes of TiXmlNode now, so there's nothing that needs to be written. It's going to take a bit to debug it - I refactored some methods for code re-use, and most classes don't have any "Clear()" method, so that is getting fixed up too.

lee

[John-Philip Leonard Johansson]

Oki very cool. Are you making the Clear() and Copy() functions pure virtual in TiXmlNode to guarantee that all nodes will have them implemented? Just curious about the implemented design. =)

/JP

[Lee Thomason]

Probably virtual but not pure virtual. Both have useful base class implementations. So there is a lot of:

TiXmlElement::Clear()
{
  TiXmlNode::Clear();
  // clear element code
}

Which has the problem, as you note, that the sub-classes can't be guarenteed to be implemented. That's a drawback - I'm putting in many many test cases for this stuff.

lee

[John-Philip Leonard Johansson]

Hmm.. you could extend the functionality with the template method [GOF95]. That is, have a public function called Clear() as you said. But make a protected ClearImpl() or ClearThis() or whatever name you think is best. The purpose is to force extension of the Clear() function.

virtual void TiXmlNode::ClearThis() = 0; // force implementation of subclass specific clearing.

void TiXmlNode::Clear() // non virtual
{
// clear code
ClearThis();
}

It's kinda a inverted way as you did in your post, but you'll have the compiler check that the subclasses have been implemented instead of testcases.

Cheers
/JP

[John-Philip Leonard Johansson]

curious if you considered my approach =)
maybe I didn't explain it to well? =/ it's also called a "hook function" by some. Clear the node in TiXmlNode::Clear() and call ClearThis() in the end, where the subclasses (element, etc) implement their own versions of ClearThis(). If a subclass doesn't have a ClearThis() you'll get a compiler error (assuming you've done ClearThis() as a pure virtual). Hope this helps =)

(ClearThis() is a dumb name... can't think of anything else though =/ )

[Lee Thomason]

Sorry, John-Philip, I should have responded to your ealier post sooner.

By the time I read the previous post, I had already implemented all the CopyTo(), Clear(), and copy/assignment operators. The code is working and I'm fairly happy with it, so I just didn't investigate other implemenations.

lee

[John-Philip Leonard Johansson]

no problem =)

as long as it's working, we're all happy ;)

/JP