[Marble-devel] Review Request 118011: [FIX] Equality operators for GeoDataPlacemark + Other issues

[Dennis Nienhüser]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118011/#review57435
-----------------------------------------------------------

Ship it!


For m_geometry comparison, your second patch looks fine. It is deleted in ~GeoDataPlacemarkPrivate which is called from ~GeoDataFeature because the d-pointer is shared in the class hierarchy. Keeping code complexity low wasn't high on the priority list of geodata I'm afraid :-P


- Dennis Nienhüser


On May 5, 2014, 9:16 p.m., Cruceru Calin-Cristian wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118011/
> -----------------------------------------------------------
> 
> (Updated May 5, 2014, 9:16 p.m.)
> 
> 
> Review request for Marble.
> 
> 
> Repository: marble
> 
> 
> Description
> -------
> 
> The previous implementation of operator== in GeoDataPlacemark was, as even the documentation said, comparing the 2 private pointers of each instance of the class. I thought that this implementation would be more appropriate, but there are still some issues, especially caused by the m_geometry pointer:
> - how should this be compared to the other's m_geometry?
> - even though it is not regarded to what for I opened this bug review request, we should think of that //FIXME comment within the GeoDataPlacemark( const GeoDataPlacemark &other ) constructor because I think it will cause problems when we will finish implementing operators== and we will start testing on real .kml files.
> - shouldn't m_geometry be deleted within the destructor, since its parent is the current object? I see that it is deleted when set a new one geometry, but not in destructor; maybe I didn't understand this correctly.
> 
> 
> Diffs
> -----
> 
>   src/lib/marble/geodata/data/GeoDataPlacemark.h bd32b4c 
>   src/lib/marble/geodata/data/GeoDataPlacemark.cpp 5df534d 
> 
> Diff: https://git.reviewboard.kde.org/r/118011/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Cruceru Calin-Cristian
> 
>

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