Hi! On Fri, Aug 16, 2013 at 11:13:38AM +0200, Johannes Kolb wrote: > in the last days I've written some tests for the objects defined in > osmium/osm/*.hpp. > Again, I will send a pull request with this changes. > > I also want to share some remarks/proposals with you: > > osmium/osm/object.hpp(235): > Is there a reason, why set_attribute() can't set the endtime attribute?
Basically set_attribute() only exists for setting attributes from the XML reader. Nobody uses it apart from that. And the endtime attribute is never in any XML file. In fact, adding the endtime attribute (for one special use case) was probably not a good idea. It will not be in the new Osmium version. > osmium/osm/node.hpp(98): > I wonder if it is a good idea to overload the comparison of > shared_ptrs. At > the moment you have to be very careful, if you want to compare > shared_ptrs to > Nodes, since the comparsion is only overloaded for shared_ptr<Node > const>. > If you have shared_ptr<Node> for example, they are compared by the > default > comparison (which is probably by memory location). > > I would recommend to remove the comparison operator for shared_ptrs. You > can compare shared_ptrs by dereferencing them (*lhs < *rhs) anyway. It is a bit odd. I think I added this so that we can put those shared_ptr in STL containers that need ordering. The new Osmium doesn't use shared_ptr for objects any more, so it doesn't matter much. > osmium/osm/way_node.hpp(82): > For WayNode objects only the comparison lhs < rhs works, > lhs > rhs gives a compiler error. > Perhaps it is necessary to implement operator> too? WayNode should probably inherit from boost::less_than_comparable like other classes do. > osmium/osm/way.hpp(77): > In the comment for add_note you write > "Will throw a range error if the way already has max_nodes_in_way > nodes." > This is nowhere implemented; not even is max_nodes_in_way defined > somewhere. The comment is wrong. I removed that restriction because, while today OSM doesn't allow ways with more than 2000 nodes, historically it was allowed. So if you work with historical data (which Osmium can do), this restriction can't be applied. (Ideally there would be some kind of setting somewhere so the programmer can chose whether they want to work with or without this restriction.) > osmium/osm/way_node_list.hpp(111): > According to the documentation at > http://www.cplusplus.com/reference/vector/vector/front/ > the front and back methods of std::vector objects are undefined, if the > vector is empty. Therefore also the methods front(), back(), is_closed() > have undefined behaviour on empty WayNodeListst. > I hope this doesn't cause problems. We should document these calls as undefined in that case also. I don't want to add checks for each and every one of them adding to the run-time cost. > osmium/osm/bounds.hpp(60): > Is there a reason, that no non-const getter for bottom_left and > top_right is implemented? > If you want to change the bounds you can only use the extend() method. This is just one of those "lets implement what I need right now and leave the rest for later"-cases. There are some more of those in Osmium. I tend to only implement what I need because adding something to a library is much easier than removing it. I don't want to add things only to find later that the interface wasn't quite right and I have to change it possibly breaking user code. That being said, simple getter and setter functions should be standard normally... Thanks for looking into these things. Some of them should be fixed probably and I welcome patches. Others we might just leave, because they might break existing user code. I am working on a new version of Osmium that changes a lot of the internal design, so some things don't apply there any more anways. I will write a proper announcement soon, but if anybody is interested, the new version of Osmium is at https://github.com/osmcode/libosmium . Jochen -- Jochen Topf [email protected] http://www.jochentopf.com/ +49-721-388298 _______________________________________________ dev mailing list [email protected] http://lists.openstreetmap.org/listinfo/dev
