mapbox / minjur

Osmium-based converter of OSM data to GeoJSON
ISC License
59 stars 14 forks source link

--attr-prefix flags #14

Closed rclark closed 8 years ago

rclark commented 8 years ago

Todo:

fixes #11


@joto I am very new to writing c++ so please critique everything about this. In addition here are some specific questions that I had:

springmeyer commented 8 years ago

@rclark did you mean to check in the d.diff file?

rclark commented 8 years ago

derp! Definitely did not mean to do that. Rebased it away -- thanks @springmeyer.

joto commented 8 years ago

@rclark Thanks. Generally this looks very good. You already indentified most of the issues yourself and I have tried to answer them in the inline comments. I have been extra picky with my comments, because I think that way you can learn fastest. Ping me on chat if you need extra explanations of those tricky C++ details like the initialization of the class membery variables. That's pretty unintuitive stuff if you have never seen it before, but will become second nature after a while.

rclark commented 8 years ago

I ended up getting the static_cast to work as follows:

static_cast<const osmium::Area&>(object).from_way()

Needed the const, as you suggested @joto, but also needed the &. My "clue" was from http://en.cppreference.com/w/cpp/language/static_cast:

If new_type is a pointer or reference to some class D and the type of expression is a pointer or reference to its non-virtual base B, static_cast performs a downcast.

Saw "reference" and thought "maybe I need an &", but honestly I can't say I entirely understand why this was successful. Is this just about making a 100% match to the way that object was defined (as const osmium::OSMObject&)?


At this point, aside from the fact that this code still concatenates strings on each .add_properties call, I think I've hit all you points. Thanks for the review and maybe we can merge now?

joto commented 8 years ago

@rclark The static_cast allows you to pretend some object is of some other type. But it doesn't just allow any kind of pretending, just some things that are, if done properly, safe and make sense. In this case the original object is of type const OSMObject& (a reference to a constant OSMObject). You can only cast this to a class type that is a) a reference, b) constant, c) derives from OSMObject, so const Area& is okay. If you leave out the const you would make the object mutable, which is not allowed (there is a const_cast for that, if it is really needed, but normally you should not use that). If your original type is OSMObject& without the const, you can add one, because that is a further restriction, that's okay. But you can't remove the restriction. If you leave out the & you would make the reference into a real "value" of the object, which doesn't make any sense. Hope this clear things up.