[Marble-devel] Review Request 121362: Fixing a memory leak coming from GeometryLayerPrivate::createGraphicsItemFromGeometry()

[Dennis Nienhüser]

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



src/lib/marble/GeoGraphicsScene.cpp
<https://git.reviewboard.kde.org/r/121362/#comment49872>

    I'd rather call clear(), see below



src/lib/marble/GeoGraphicsScene.cpp
<https://git.reviewboard.kde.org/r/121362/#comment49871>

    This is not complete as m_features has references to m_items subparts still.
    
    I'd suggest a different approach:
    - Merge the functionality of the existing GeoGraphicsScene::eraseAll() into GeoGraphicsScene::clear() and remove GeoGraphicsScene::eraseAll() in favor of that method
    - move the clear() implementation into the private class and also call it from its destructor
    
    This minimizes code duplication and makes sure to clean up consistently. Also gets rid of the wrong doxygen comments of eraseAll.



src/lib/marble/MarbleModel.cpp
<https://git.reviewboard.kde.org/r/121362/#comment49870>

    Good catch, but the fix does not look complete to me. Can you please also do
    
    - in MarbleModel::setLegend(), delete any previously set legend
    - in MarbleModel.h add a doxygen comments to setLegend() that says that MarbleModel takes ownership of the passed instance
    - in MarbleLegendBrowser.cpp fix the handling of m_isLegendLoaded. Move the checks for it inside loadLegend(): At the start of the method check whether the legend was loaded already by querying m_isLegendLoaded. If true, return. Otherwise, set m_isLegendLoaded to true (that is nowhere done currently) and actually load it.
    
    Please also check for potential regressions (is the legend still loaded correctly afterwards on theme changes?)


- Dennis Nienhüser


On Dec. 6, 2014, 9:04 p.m., Illya Kovalevskyy wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/121362/
> -----------------------------------------------------------
> 
> (Updated Dec. 6, 2014, 9:04 p.m.)
> 
> 
> Review request for Marble, Dennis Nienhüser and Torsten Rahn.
> 
> 
> Repository: marble
> 
> 
> Description
> -------
> 
> Related GCI-2014 task: http://www.google-melange.com/gci/task/view/google/gci2014/5790648552652800
> 
> Valgrind detected a memory leak for approx 2k bytes in GeometryLayerPrivate::createGraphicsItemFromGeometry() function. That method adds newly created GeoGraphicsItem to the GeoGraphicsScene instance, which doesn't take care of newly created object. I tweaked code a bit so it cleans all items added.
> 
> Leak has been neutralized.
> 
> 
> Diffs
> -----
> 
>   src/lib/marble/GeoGraphicsScene.cpp 390d868 
>   src/lib/marble/MarbleModel.cpp c71c9cc 
> 
> Diff: https://git.reviewboard.kde.org/r/121362/diff/
> 
> 
> Testing
> -------
> 
> Double valgrind-check (inb4 the fix and after)
> 
> 
> Thanks,
> 
> Illya Kovalevskyy
> 
>

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