Review Request: KStars: fixed code checker issues #2

[Kevin Krammer]

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



kstars/dialogs/detaildialog.cpp
<http://git.reviewboard.kde.org/r/108192/#comment19016>

    I would recommend we revert this change.
    This is a very broken method and needs some serious rework.
    I.e. later on around line 715 it starts to add child widgets to the dialog but allocates them on the function's stack.



kstars/dialogs/detaildialog.cpp
<http://git.reviewboard.kde.org/r/108192/#comment19017>

    if we revert as recommended above this won't be necessary either



kstars/kstarsactions.cpp
<http://git.reviewboard.kde.org/r/108192/#comment19014>

    just
    dlg->exec();
    if it didn't need to check the return value before it doesn't need to do it now.
    



kstars/kstarsactions.cpp
<http://git.reviewboard.kde.org/r/108192/#comment19015>

    here on the other hand we use the change to QPointer to check if the dialog actually still exists:
    KNS3::Entry::List entries;
    if (dlg) {
      entries = dlg->installedEntries();
    }



kstars/printing/pwizfovbrowse.cpp
<http://git.reviewboard.kde.org/r/108192/#comment19018>

    right, just:
    dialog->exec();



kstars/printing/pwizfovsh.cpp
<http://git.reviewboard.kde.org/r/108192/#comment19019>

    move to the end of the include section.
    The first header to include is always the one declaring the class, in this case pwizfovsh.h as it was before



kstars/printing/pwizfovsh.cpp
<http://git.reviewboard.kde.org/r/108192/#comment19021>

    The idea of using QPointer instead is to be able to check if the dialog still exists after exec() returned.
    So leaving this without additional check is as potentially crashy as before.
    I recommend to change the previous line to
    if (findDlg->exec() == Qt::Accepted && findDlg) {
    



kstars/printing/pwizobjectselection.cpp
<http://git.reviewboard.kde.org/r/108192/#comment19022>

    move to end of include seciton



kstars/printing/pwizobjectselection.cpp
<http://git.reviewboard.kde.org/r/108192/#comment19023>

    see above



kstars/printing/pwizprint.cpp
<http://git.reviewboard.kde.org/r/108192/#comment19024>

    #include <QPointer>
    



kstars/tools/conjunctions.cpp
<http://git.reviewboard.kde.org/r/108192/#comment19025>

    this needs a pointer access check again.
    see above for findDlg



kstars/tools/modcalcvlsr.cpp
<http://git.reviewboard.kde.org/r/108192/#comment19026>

    pointer access check, see above


- Kevin Krammer


On Jan. 5, 2013, 7:48 a.m., Mohammed Nafees wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108192/
> -----------------------------------------------------------
> 
> (Updated Jan. 5, 2013, 7:48 a.m.)
> 
> 
> Review request for KDE Edu and Kevin Krammer.
> 
> 
> Description
> -------
> 
> http://www.google-melange.com/gci/task/view/google/gci2012/8192207
> 
> 
> Diffs
> -----
> 
>   kstars/dialogs/detaildialog.cpp efda624 
>   kstars/ekos/guide/gmath.cpp bf629a2 
>   kstars/ekos/guide/guider.cpp 30c6bcd 
>   kstars/ekos/guide/rcalibration.cpp 3340ae4 
>   kstars/ekos/guide/scroll_graph.cpp a37e81c 
>   kstars/kstarsactions.cpp 185507a 
>   kstars/printing/pwizfovbrowse.cpp 94edd09 
>   kstars/printing/pwizfovsh.cpp b5a4101 
>   kstars/printing/pwizobjectselection.cpp 50b9f5a 
>   kstars/printing/pwizprint.cpp 5c95318 
>   kstars/skyobjects/satellite.cpp 0ca44f7 
>   kstars/tools/conjunctions.cpp 6b8c081 
>   kstars/tools/modcalcvlsr.cpp 2a9b355 
> 
> Diff: http://git.reviewboard.kde.org/r/108192/diff/
> 
> 
> Testing
> -------
> 
> yes
> 
> 
> Thanks,
> 
> Mohammed Nafees
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-edu/attachments/20130105/9391ffb5/attachment-0001.html>