Introduce GeoDataBuilding
ClosedPublic

Authored by mnafees on Jun 14 2017, 1:50 PM.

Diff Detail

Repository
R34 Marble
Lint
Lint Skipped
Unit
Unit Tests Skipped
nienhueser added inline comments.Jun 14 2017, 6:32 PM
src/lib/marble/geodata/data/GeoDataBuilding.cpp
19

guess you want to create a new GeoDataBuildingPrivate here

24

must delete d here. Also a copy ctor and assigment operator are needed to deal with the pointer member correctly.

src/lib/marble/geodata/data/GeoDataBuilding.h
84

Let's use QVector<int> instead of a pointer to int, the latter is c-ish and error-prone.

104

Do we need a setter? If not (and GeoDataBuilding takes care of creating the instance it returns) then ownership is clear.

mnafees updated this revision to Diff 15466.Jun 15 2017, 3:25 PM
mnafees marked 4 inline comments as done.
nienhueser requested changes to this revision.Jun 15 2017, 4:50 PM
nienhueser added inline comments.
src/lib/marble/geodata/data/GeoDataBuilding_p.h
34

The pointer member here is still not handled correctly. E.g. when copying a GeoDataBuilding instance, the code currently would result in a double delete when both go out of scope. The easiest way around it would be to change it to
GeoDataMultiGeometry m_multiGeometry; and do a return &d->m_multiGeometry; in GeoDataBuilding::multiGeometry()

This revision now requires changes to proceed.Jun 15 2017, 4:50 PM
mnafees updated this revision to Diff 15473.Jun 15 2017, 5:42 PM
mnafees edited edge metadata.
mnafees marked an inline comment as done.
nienhueser requested changes to this revision.Jun 15 2017, 6:31 PM
nienhueser added inline comments.
src/lib/marble/geodata/data/GeoDataBuilding_p.h
29

Should be of double type.

This revision now requires changes to proceed.Jun 15 2017, 6:31 PM
mnafees updated this revision to Diff 15474.Jun 15 2017, 6:46 PM
mnafees edited edge metadata.
mnafees marked an inline comment as done.
nienhueser requested changes to this revision.Jun 15 2017, 8:48 PM

I miss a "sane" constructor. What about introducing a unit test so the class is used in one place at least for a basic sanity check?

This revision now requires changes to proceed.Jun 15 2017, 8:48 PM
mnafees updated this revision to Diff 15497.Jun 16 2017, 3:01 PM
mnafees edited edge metadata.
nienhueser accepted this revision.Jun 16 2017, 5:50 PM

Thanks, time to push it I'd say - Torsten?

This revision is now accepted and ready to land.Jun 16 2017, 5:50 PM