[Marble-devel] geodata-nt

[Jens-Michael Hoffmann]

Am Monday 30 March 2009 02:33:45 schrieb Patrick Spendrin:
> I just merged the marble-geodata-nt branch and I am pretty sure I
> inserted loads of bugs.

This was a lot of code which was merged apparently without any kind of review. 
I think this was not a good idea.

I had a quick look at the code and here are my comments:

1. SVN commit 946669 by sengels: add virtual desctructor to suppress warning
well, virtual destructors are not added "to supress warnings", but to prevent 
undefined behaviour (and for that reason there is a warning)

2. src/lib/geodata/data/GeoDataPolygon_p.h
The header contains the class declaration two times.

3. 
+class  GeoDataPointPrivate : public Marble::GeoDataGeometryPrivate,
+                             public Marble::GeoDataCoordinatesPrivate
+{
+  public:
+     GeoDataPointPrivate()
+    {
+    }

should call base class constructors.

4. src/lib/geodata/data/GeoDataPoint_p.h
contains class declaration two times.

5. src/lib/geodata/data/GeoDataLinearRing_p.h
contains class declaration two times.

6. src/lib/geodata/data/GeoDataFeature_p.h
contains class declaration two times, implementations differ

7. src/lib/geodata/data/GeoDataFeature_p.h
assignment operator copies pointer members, if this is ok, please document 
why.
+    GeoDataStyle* m_style;
+    GeoDataStyleMap* m_styleMap;

8. src/lib/geodata/data/tests/CopyTest.cpp
file content duplicated.

9. src/lib/geodata/data/GeoDataLineString_p.h
file content duplicated.


well, at this point I gave up.

I'd like to suggest to do the following:
- revert this merge
- fix the issues described above
- post resulting changes for review
- merge once all issues arising from review have been addressed.


Best Regards

Jens-Michael



> So if you come across a new one, just try to fix it, they might be
> really easy.
> You can safely commit to marble-trunk again now.
>
> regards,
> Patrick