[Marble-devel] Review Request: Extension for the .osm file parser.

Tue Aug 14 10:05:23 UTC 2012

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106007/#review17361
-----------------------------------------------------------

Looks good, just found some memory leaks and some nitpicking to get the code shorter and adhere to coding style guidelines.

src/plugins/runner/osm/handlers/OsmMemberTagHandler.cpp
<http://git.reviewboard.kde.org/r/106007/#comment13553>

    No abbreviation for this variable please, use polygon instead of s.

src/plugins/runner/osm/handlers/OsmMemberTagHandler.cpp
<http://git.reviewboard.kde.org/r/106007/#comment13554>

    No abbreviation for this variable please, use line instead of l (see http://techbase.kde.org/Policies/Kdelibs_Coding_Style#Variable_declaration)

src/plugins/runner/osm/handlers/OsmMemberTagHandler.cpp
<http://git.reviewboard.kde.org/r/106007/#comment13545>

    I think you can simplify this (making it faster) and replace the for loop with a call to

    envelope << *l;

    Or, given that in this case envelope is empty, also

    envelope = *l;

src/plugins/runner/osm/handlers/OsmMemberTagHandler.cpp
<http://git.reviewboard.kde.org/r/106007/#comment13546>

    Should be identical to the (more common and easier to read)
    l->first() == envelope.first()

    Same for the code below

src/plugins/runner/osm/handlers/OsmMemberTagHandler.cpp
<http://git.reviewboard.kde.org/r/106007/#comment13547>

    There's a memory leak here, temp is never deleted. Could be created on the stack instead.

    Generally I think the reversion of the linestring could be done more elegantly in-place like this:

    int const size = envelope.size();
    for ( int i=0; i<size; ++i ) {
      qSwap( envelope[i], envelope[size-i] );
    }

    Same below in case 3.

src/plugins/runner/osm/handlers/OsmMemberTagHandler.cpp
<http://git.reviewboard.kde.org/r/106007/#comment13548>

    Couldn't this be simplified to (using operator << is faster)

    envelope.remove( envelope.size() - 1 );
    envelope << *l;

src/plugins/runner/osm/handlers/OsmMemberTagHandler.cpp
<http://git.reviewboard.kde.org/r/106007/#comment13550>

    Couldn't this be simplified to (using operator << is faster)

    envelope.remove( envelope.size() - 1 );
    envelope << *l;

src/plugins/runner/osm/handlers/OsmMemberTagHandler.cpp
<http://git.reviewboard.kde.org/r/106007/#comment13551>

    Another memory leak here. Please do the inversion like above.

src/plugins/runner/osm/handlers/OsmMemberTagHandler.cpp
<http://git.reviewboard.kde.org/r/106007/#comment13552>

    Better is

    envelope << l->at( x );

    as Thibaut already pointed out.

    Same for case 4.

src/plugins/runner/osm/handlers/OsmMemberTagHandler.cpp
<http://git.reviewboard.kde.org/r/106007/#comment13555>

    Memory leak again, and could possibly be simplified to 

    s->appendInnerBoundary( GeoDataLinearRing( *l ) );

src/plugins/runner/osm/handlers/OsmRelationFactory.h
<http://git.reviewboard.kde.org/r/106007/#comment13556>

    We try to follow Qt-style API where the getter for the member foo is not prefixed get (i.e. foo() instead of getFoo()), so I'd prefer

    static GeoDataPolygon polygon( quint64 id );

src/plugins/runner/osm/handlers/OsmRelationFactory.h
<http://git.reviewboard.kde.org/r/106007/#comment13560>

    Wouldn't clear() be a more proper name?

src/plugins/runner/osm/handlers/OsmRelationFactory.cpp
<http://git.reviewboard.kde.org/r/106007/#comment13557>

    I guess this can be removed? Who owns the polygons, who cleans them up?

    To me it looks like OsmRelationTagHandler.cpp adds them both to the factory and the document, so the document owns them and will delete them as needed, hence there is no cleanup needed here (except for the clear() call as done)

src/plugins/runner/osm/handlers/OsmRelationTagHandler.h
<http://git.reviewboard.kde.org/r/106007/#comment13558>

    Add yourself here?

src/plugins/runner/osm/handlers/OsmRelationTagHandler.cpp
<http://git.reviewboard.kde.org/r/106007/#comment13559>

    Add yourself here?

src/plugins/runner/osm/handlers/OsmWayFactory.h
<http://git.reviewboard.kde.org/r/106007/#comment13561>

    One line should be you, not twice Konstantin

src/plugins/runner/osm/handlers/OsmWayFactory.h
<http://git.reviewboard.kde.org/r/106007/#comment13562>

    line(), see above

src/plugins/runner/osm/handlers/OsmWayFactory.h
<http://git.reviewboard.kde.org/r/106007/#comment13563>

    clear(), see above

src/plugins/runner/osm/handlers/OsmWayFactory.cpp
<http://git.reviewboard.kde.org/r/106007/#comment13564>

    The commented code can probably be removed, ownership seems like above

- Dennis Nienhüser

On Aug. 14, 2012, 9:54 a.m., Ander Pijoan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106007/
> -----------------------------------------------------------
> 
> (Updated Aug. 14, 2012, 9:54 a.m.)
> 
> 
> Review request for Marble.
> 
> 
> Description
> -------
> 
> Extension for the .osm file parser. Relations can be built with different ways (for example for describing a building with a hole in it) tagged as members.
> 
> So with this diff, <member> tag is parsed and ways are stored for then a relation to have them available if necessary.
> 
> Moreover a relation can also contain another relation, so a relation factory has also been created to store them.
> 
> 
> Diffs
> -----
> 
>   src/plugins/runner/osm/CMakeLists.txt 6e92624 
>   src/plugins/runner/osm/OsmParser.cpp 5285566 
>   src/plugins/runner/osm/handlers/OsmElementDictionary.h 2d40f6b 
>   src/plugins/runner/osm/handlers/OsmElementDictionary.cpp c99d4a9 
>   src/plugins/runner/osm/handlers/OsmGlobals.cpp 0b5ea25 
>   src/plugins/runner/osm/handlers/OsmMemberTagHandler.h PRE-CREATION 
>   src/plugins/runner/osm/handlers/OsmMemberTagHandler.cpp PRE-CREATION 
>   src/plugins/runner/osm/handlers/OsmNodeFactory.h b9c6580 
>   src/plugins/runner/osm/handlers/OsmNodeFactory.cpp c51e373 
>   src/plugins/runner/osm/handlers/OsmNodeTagHandler.cpp f433a5d 
>   src/plugins/runner/osm/handlers/OsmOsmTagHandler.cpp 997f628 
>   src/plugins/runner/osm/handlers/OsmRelationFactory.h PRE-CREATION 
>   src/plugins/runner/osm/handlers/OsmRelationFactory.cpp PRE-CREATION 
>   src/plugins/runner/osm/handlers/OsmRelationTagHandler.h PRE-CREATION 
>   src/plugins/runner/osm/handlers/OsmRelationTagHandler.cpp PRE-CREATION 
>   src/plugins/runner/osm/handlers/OsmTagTagHandler.cpp 7a8f432 
>   src/plugins/runner/osm/handlers/OsmWayFactory.h PRE-CREATION 
>   src/plugins/runner/osm/handlers/OsmWayFactory.cpp PRE-CREATION 
>   src/plugins/runner/osm/handlers/OsmWayTagHandler.cpp fdcd2a8 
> 
> Diff: http://git.reviewboard.kde.org/r/106007/diff/
> 
> 
> Testing
> -------
> 
> Testing done with some .osm files with different ways of representing buildings and using relations. Works OK.
> 
> 
> Screenshots
> -----------
> 
> Osm parser test
>   http://git.reviewboard.kde.org/r/106007/s/675/
> 
> 
> Thanks,
> 
> Ander Pijoan
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/marble-devel/attachments/20120814/cabb43db/attachment-0001.html>