I don't see any derived class overriding this function, so there is no point in making it virtual. On the other hand if it is ever called in a context where the compiler cannot determine the type of the TiXmlNode at compile time, you lose the benefit of the inlining, because the compiler must generate a virtual function call.
You could make it non-virtual and turn it into a trivially inlined accessor function.
Alternatively you could leave it virtual, or possibly pure virtual, and override it for each derived class to return the appropriate NodeType. Along with some other measures, such as rewriting the downcasting functions, this approach would probably enable you to eliminate the type altogether as a data member.
Neither approach would require any changes in the client code.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Looking at it now I tend to think its a mistake thats virtual, probably a leftover.
I think you're right to either make it a "plain old" inline function returning the value, or don't store the value, and instead require each subclass to override.
My preference is just delete 'virtual' from the declaration. My rationale for this (and I can see both sides) is that (a) its simple (b) its effective; (c) the number of subclasses won't grow to include previously unthought of classes - its always going to be the classes there, so overriding it is not useful. (d) given the previous, there's no point paying the 'price' (whatever it is) of a virtual function call.
Although its true there are numerous "nice" OO things that could be done using virtual etc, I reckon just delete the 'virtual' from the function and its good.
WDYT?
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Why is TiXmlNode::Type() both virtual and inline?
I don't see any derived class overriding this function, so there is no point in making it virtual. On the other hand if it is ever called in a context where the compiler cannot determine the type of the TiXmlNode at compile time, you lose the benefit of the inlining, because the compiler must generate a virtual function call.
You could make it non-virtual and turn it into a trivially inlined accessor function.
Alternatively you could leave it virtual, or possibly pure virtual, and override it for each derived class to return the appropriate NodeType. Along with some other measures, such as rewriting the downcasting functions, this approach would probably enable you to eliminate the type altogether as a data member.
Neither approach would require any changes in the client code.
Looking at it now I tend to think its a mistake thats virtual, probably a leftover.
I think you're right to either make it a "plain old" inline function returning the value, or don't store the value, and instead require each subclass to override.
My preference is just delete 'virtual' from the declaration. My rationale for this (and I can see both sides) is that (a) its simple (b) its effective; (c) the number of subclasses won't grow to include previously unthought of classes - its always going to be the classes there, so overriding it is not useful. (d) given the previous, there's no point paying the 'price' (whatever it is) of a virtual function call.
Although its true there are numerous "nice" OO things that could be done using virtual etc, I reckon just delete the 'virtual' from the function and its good.
WDYT?
Ellers is correct - it's just a bug of the "why did I type that?" variety.
Bug filed.
lee