[OSM-dev] Osmium: more tests and some questions

Jochen Topf jochen at remote.org
Fri Aug 16 09:45:27 UTC 2013


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  jochen at remote.org  http://www.jochentopf.com/  +49-721-388298



More information about the dev mailing list