While making a patch for TinyXML I noticed some stuff that had me wondering, I'm not trying to be Mr Coder or something. I'm just curious as I know that some things just sound good on paper but not in practice, and what I know is from paper. Don't have much experience...
Questions about the TiXmlBase class.
-----
/// Appends the XML node or attribute to a std::string.
friend std::string& operator<< (std::string& out, const TiXmlNode& base );
Isn't that a little odd behaviour? No one ever does std::string << something, so no one would expect this functionality?
Maybe a
std::string toString() const; // user can then: mystring.append( xml.toString() );
or
void appendTo( std::string& str ) const;
instead?
-----
void operator=( const TiXmlNode& base ); // not allowed.
why not just void operator=( const TiXmlNode& ) {} // not allowed. ?
-----
for ( node = elm.firstChild; node; node = node->NextSibling() )
why firstChild? instead of using a function like for NextSibling()? Because if the implementation change, this will brake..?
-----
why is
// [internal use] Creates a new Element and returns it.
virtual TiXmlNode* Clone() const;
// [internal use]
"internal"? And yet declared in the public interface?
It's a common prototype pattern function, it _should_ be public?
-----
it would be nice with a internal version control, so one knows what version one has of TinyXml, it can be easily added with $Id$ in the .h and .cpp files. The way we do it in our project is to add this to the top of all our files:
/*$Id: BindXMLIO.h,v 1.1 2004/05/22 18:45:03 seriema Exp $
Abunai Productions, 2004, all rights reserved.
[ what ]: Loading/Saving helper functions for some types, by parsing xml with TinyXML.
Use the macros for safer error loggin, because they include caller location description.
[ auth ]: John-Philip "Seriema" Johansson
[e-mail]: seriema@bredband.net
*/
The "BindXMLIO.h,v 1.1 2004/05/22 18:45:03 seriema Exp" is automaticly added by the CVS where you write $Id$
Got an idea, there could be a TiXmlBase::Version() that returns the version. I think the version "v 1.1" can be gotten with a CVS command, like $Version$ or something (I can check this out and send a patch if you want). The point is for people to stamp their xml files, but that maybe is unnecessary info for the xml file?
-----
The base class TiXmlNode has a virtual Clone() that returns a TiXmlNode*, the prototype pattern is great. But C++ allows you to subclass the return type too, meaning that for example: TiXmlElement::Clone() can return a TiXmlElement* instead of a TiXmlNode*. I learned this in the book Effective C++ by Scott Meyers.
-----
Thanx =)
/JP
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
1. The std::string << operator
Does seem a little unusual, but not harmful. It could be handy.
2. operator=
I think you're right.
3. firstChild vs FirstChild()
I think you're right again.
4. internal Clone()
Yep, prob also right.
5. Version control history in the file.
(Rant warning...)
This is ony I *really* don't agree on.
Whenever I'm maintaining code with that crap in the header I strip it out. CVS (or whatever control system) is the place to keep that information, not the file itself. Putting it in the file is useless; when I want to see what version I'm using I'll ask CVS.
6. TiXmlBase::Version()
Might be handy. Either as compile-time (ie #defines) or run-time (ie consts). In either case it would be handy to have major and minor versions. I guess one reason to have it there is to test for UTF-8 support, but even then, is it really needed?
Ellers
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
1. As you said, not harmful. And it could be handy. That's why I proposed to keep the functionality, but change the function. To something more expected.
2-4. Yeah thanx =)
5. Hehe, well I'm kinda new to CVS. But when our group is together and program, it's super convinient to just look over ones shoulder to check the version when a bug shows up. But I understand your point.
My only beef is... I actually don't know what version of TinyXML I have, I named the folder "TinyXML" and it isn't taken from the CVS. So... where's the version number?..
6. Yes I thought about how "needed" it really is. Maybe not at all. But it could come in handy, maybe =) If the file's gonna have a version, why not just put it in a place where people can access it just in case? Haven't thought about UTF-8 support but it might be good for it?
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
When I downloaded TinyXml the first time it came packaged in a tinyxml-x.y.tar.gz (or similar) file, and it unpacked to tinyxml-x.y so the version was right there.
When I downloaded it straight from CVS it (naturally) doesn't have any version, because by definition it doesn't have a version if you're getting it straight from CVS.
As for whether its really needed... I'm not sure. I'm inclined to take the line from the movie "Ronin" and say "if there is any doubt, there is no doubt". In other words, if we can't really think of a reason for a version number, forget it until there is a reason.
As for UTF-8 support, see other threads in the tinyxml forums ;)
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
#1 << to string, was crazy requested. That is one of the more popular features I've added. Especially with 2 lines of code. It is useful - I wish output to string could be done in a standard way in the non-STL branch.
#2 - they are equivalent. They won't compile.
#3 - context? where is that in the code?
#4 [internal]. C++ does not, of course, have package scope. The problem is siblings. A document is a sibling, inheritence wise, of an element. But they still need to share functionality that is not actually part of the public API.
There is no simple work-around in C++. Java provides package scope for that issue.
#5 Versioning via CVS. I am firmly it support of Ellers rant. I *hate* that at the top of files.
#6 Versioning in the product. Good idea, actually. It would be useful for bug reports if nothing else.
lee
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
For item 4, I definitely get where Lee is coming from - package scope in Java is intuitive and useful. But given we're in C++ this sort of thing can still be modeled effectively - just not as cleanly ;) But for the effort.. its fine as it is.
I reckon remove the "internal use" comment. Just let it be unashamedly public.
FWIW
Ellers
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
#2, actually they aren't equal. The first version does two things
A) the function isn't implemented anywhere
B) it uses an unreferenced parameter
But as you said, if you try to use it it won't compile.
#3, on my latest version of tinyxml.cpp (revision 1.57) it's on line 671, in the function TiXmlElement::Clone(). I found it during my patch-making for the Copy implementation, so that piece of code is in TiXmlElement::Copy() in my patch. A bit of searching gave me that exact piece of code used here:
TiXmlElement::Print()
TiXmlElement::StreamOut()
TiXmlDocument::Clone()
#4, I think I understand. Just found it odd =) Especially since Clone() is a handy function that can be used by the user. And thus doesn't have to be "internal".
#6, that would be great!
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
- introduced a version #
- cleaned up the improper "internals"
- added some docs
- left the firstChild/FirstChild thing...felt guilty changing working code at 11pm.
lee
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
While making a patch for TinyXML I noticed some stuff that had me wondering, I'm not trying to be Mr Coder or something. I'm just curious as I know that some things just sound good on paper but not in practice, and what I know is from paper. Don't have much experience...
Questions about the TiXmlBase class.
-----
/// Appends the XML node or attribute to a std::string.
friend std::string& operator<< (std::string& out, const TiXmlNode& base );
Isn't that a little odd behaviour? No one ever does std::string << something, so no one would expect this functionality?
Maybe a
std::string toString() const; // user can then: mystring.append( xml.toString() );
or
void appendTo( std::string& str ) const;
instead?
-----
void operator=( const TiXmlNode& base ); // not allowed.
why not just void operator=( const TiXmlNode& ) {} // not allowed. ?
-----
for ( node = elm.firstChild; node; node = node->NextSibling() )
why firstChild? instead of using a function like for NextSibling()? Because if the implementation change, this will brake..?
-----
why is
// [internal use] Creates a new Element and returns it.
virtual TiXmlNode* Clone() const;
// [internal use]
"internal"? And yet declared in the public interface?
It's a common prototype pattern function, it _should_ be public?
-----
it would be nice with a internal version control, so one knows what version one has of TinyXml, it can be easily added with $Id$ in the .h and .cpp files. The way we do it in our project is to add this to the top of all our files:
/*$Id: BindXMLIO.h,v 1.1 2004/05/22 18:45:03 seriema Exp $
Abunai Productions, 2004, all rights reserved.
[ what ]: Loading/Saving helper functions for some types, by parsing xml with TinyXML.
Use the macros for safer error loggin, because they include caller location description.
[ auth ]: John-Philip "Seriema" Johansson
[e-mail]: seriema@bredband.net
*/
The "BindXMLIO.h,v 1.1 2004/05/22 18:45:03 seriema Exp" is automaticly added by the CVS where you write $Id$
Got an idea, there could be a TiXmlBase::Version() that returns the version. I think the version "v 1.1" can be gotten with a CVS command, like $Version$ or something (I can check this out and send a patch if you want). The point is for people to stamp their xml files, but that maybe is unnecessary info for the xml file?
-----
The base class TiXmlNode has a virtual Clone() that returns a TiXmlNode*, the prototype pattern is great. But C++ allows you to subclass the return type too, meaning that for example: TiXmlElement::Clone() can return a TiXmlElement* instead of a TiXmlNode*. I learned this in the book Effective C++ by Scott Meyers.
-----
Thanx =)
/JP
Some comments, for what its worth...
1. The std::string << operator
Does seem a little unusual, but not harmful. It could be handy.
2. operator=
I think you're right.
3. firstChild vs FirstChild()
I think you're right again.
4. internal Clone()
Yep, prob also right.
5. Version control history in the file.
(Rant warning...)
This is ony I *really* don't agree on.
Whenever I'm maintaining code with that crap in the header I strip it out. CVS (or whatever control system) is the place to keep that information, not the file itself. Putting it in the file is useless; when I want to see what version I'm using I'll ask CVS.
6. TiXmlBase::Version()
Might be handy. Either as compile-time (ie #defines) or run-time (ie consts). In either case it would be handy to have major and minor versions. I guess one reason to have it there is to test for UTF-8 support, but even then, is it really needed?
Ellers
I like all comments so feel free to write =)
1. As you said, not harmful. And it could be handy. That's why I proposed to keep the functionality, but change the function. To something more expected.
2-4. Yeah thanx =)
5. Hehe, well I'm kinda new to CVS. But when our group is together and program, it's super convinient to just look over ones shoulder to check the version when a bug shows up. But I understand your point.
My only beef is... I actually don't know what version of TinyXML I have, I named the folder "TinyXML" and it isn't taken from the CVS. So... where's the version number?..
6. Yes I thought about how "needed" it really is. Maybe not at all. But it could come in handy, maybe =) If the file's gonna have a version, why not just put it in a place where people can access it just in case? Haven't thought about UTF-8 support but it might be good for it?
When I downloaded TinyXml the first time it came packaged in a tinyxml-x.y.tar.gz (or similar) file, and it unpacked to tinyxml-x.y so the version was right there.
When I downloaded it straight from CVS it (naturally) doesn't have any version, because by definition it doesn't have a version if you're getting it straight from CVS.
As for whether its really needed... I'm not sure. I'm inclined to take the line from the movie "Ronin" and say "if there is any doubt, there is no doubt". In other words, if we can't really think of a reason for a version number, forget it until there is a reason.
As for UTF-8 support, see other threads in the tinyxml forums ;)
Ack - 6 threads at once. :)
#1 << to string, was crazy requested. That is one of the more popular features I've added. Especially with 2 lines of code. It is useful - I wish output to string could be done in a standard way in the non-STL branch.
#2 - they are equivalent. They won't compile.
#3 - context? where is that in the code?
#4 [internal]. C++ does not, of course, have package scope. The problem is siblings. A document is a sibling, inheritence wise, of an element. But they still need to share functionality that is not actually part of the public API.
There is no simple work-around in C++. Java provides package scope for that issue.
#5 Versioning via CVS. I am firmly it support of Ellers rant. I *hate* that at the top of files.
#6 Versioning in the product. Good idea, actually. It would be useful for bug reports if nothing else.
lee
Cool. So "closure" on these points...
1. Good as-is, no action required.
2. Good as-is, no action required.
3. More info from JP if he's interested...
4. Hmm. Comments below.
5. Solved ;)
6. To be implemented.
For item 4, I definitely get where Lee is coming from - package scope in Java is intuitive and useful. But given we're in C++ this sort of thing can still be modeled effectively - just not as cleanly ;) But for the effort.. its fine as it is.
I reckon remove the "internal use" comment. Just let it be unashamedly public.
FWIW
Ellers
oki then =)
#2, actually they aren't equal. The first version does two things
A) the function isn't implemented anywhere
B) it uses an unreferenced parameter
But as you said, if you try to use it it won't compile.
#3, on my latest version of tinyxml.cpp (revision 1.57) it's on line 671, in the function TiXmlElement::Clone(). I found it during my patch-making for the Copy implementation, so that piece of code is in TiXmlElement::Copy() in my patch. A bit of searching gave me that exact piece of code used here:
TiXmlElement::Print()
TiXmlElement::StreamOut()
TiXmlDocument::Clone()
#4, I think I understand. Just found it odd =) Especially since Clone() is a handy function that can be used by the user. And thus doesn't have to be "internal".
#6, that would be great!
Sounds good - I'll get these points cleaned up for the next release.
lee
Okay:
- introduced a version #
- cleaned up the improper "internals"
- added some docs
- left the firstChild/FirstChild thing...felt guilty changing working code at 11pm.
lee