WindowPosition::saveToNode()

[OrbWeaver]

I noticed while engaged in the PersistentTransientWindow refactor that WindowPosition has the following method:

 

void WindowPosition::saveToNode(xml::Node node) {
node.setAttributeValue("xPosition", intToStr(_position[0]));
node.setAttributeValue("yPosition", intToStr(_position[1]));
node.setAttributeValue("width", intToStr(_size[0]));
node.setAttributeValue("height", intToStr(_size[1]));
}

 

At first glance, this method appears to do absolutely nothing, since the xml::Node is passed by value not by reference and therefore the changes are only made to a temporary copy.

 

However, I guess this still works because xml::Node is just a wrapper for an xmlNode* pointer, which will still point to the same xmlNode even when the xml::Node object is copied. Was this the intention? It seems dangerous to me because there is no guarantee that copying an xml::Node will still point to the same xmlNode, even if it does at present.

 

(For absolute correctness, xml::Node should have a copy-constructor defined which performs a deep copy of the underlying structures, so that copy semantics work as expected, but this is probably not worth it in reality).

[greebo]

I certainly didn't intend the xml::Node to be passed by value, thanks for catching that one. Yes, it works because the contained pointer is still the same, that's probably why I didn't notice this.

 

Could you change it to take a reference or should I do it?

[OrbWeaver]

I've changed it in my branch, but it is still as written above in the trunk. There is no need to change it separately, since it works for now and (hopefully) my changes can be merged back soon.