[Kde-pim] Review Request: fix for some cppcheck and clang errors

[Kevin Krammer]

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



akonadi_next/kreparentingproxymodel.cpp
<http://git.reviewboard.kde.org/r/105149/#comment11375>

    I guess it wouldn't hurt to initialize parentId here as well



akregator/src/subscriptionlistview.cpp
<http://git.reviewboard.kde.org/r/105149/#comment11376>

    I am not sure about this.
    if this is never called with 0 then an assert would make more sense. if it is called with 0, it should probably be propagated to the base class



calendarsupport/next/incidencechanger2.cpp
<http://git.reviewboard.kde.org/r/105149/#comment11377>

    Yes and no.
    The idea of QLatin1String is to make conversion into QString's internal 16bit unicode faster by specifying that the input does only contain latin1 characters.
    So the equivalent would be QString::fromLatin1( ... ).arg( ... );
    
    However, this looks very much like a string that would show up in the user interface somewhere, in which case this should be one of the i18n() call family.
    
    Can you check where this value ends up at?



kaddressbook/printing/mikesstyle.cpp
<http://git.reviewboard.kde.org/r/105149/#comment11378>

    better put some space inside the new parentheses (coding style)
    ( counter % 2 )



kalarm/lib/spinbox.cpp
<http://git.reviewboard.kde.org/r/105149/#comment11379>

    space after ( and before ), see above



kjots/kjotsbrowser.cpp
<http://git.reviewboard.kde.org/r/105149/#comment11380>

    this looks plausible but it changes the condition. is there any bug associated with that?



kleopatra/utils/gnupg-helper.cpp
<http://git.reviewboard.kde.org/r/105149/#comment11381>

    also plausible but again changes the condition



knode/foldertreewidget.cpp
<http://git.reviewboard.kde.org/r/105149/#comment11382>

    I think it is expected that tree is not 0 here, so better make that an assert.
    


- Kevin Krammer


On June 4, 2012, 6:28 p.m., Jaime Torres Amate wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105149/
> -----------------------------------------------------------
> 
> (Updated June 4, 2012, 6:28 p.m.)
> 
> 
> Review request for KDEPIM.
> 
> 
> Description
> -------
> 
> 1. change initialization order to follow variable declaration order
> 2. initialize the variable
> 3. do nothing if m is null.
> 4. adding a const bool to a const char* does not appends the value. Changed to QString with arg
> 5. add parenthesis to clarify ? expressions priority
> 6. add parenthesis to clarify ? expressions priority
> 7. add parenthesis to clarify || vs && priority
> 8. i++ to ++i (little faster)
> 9. add parenthesis to clarify || vs && priority
> 10. i++ to ++i (little faster)
> 11. avoid using tree if tree is null
> 12. avoid using extern "C" if the function return a class, incompatible C linkage (the comments are already removed)
> 
> 
> Diffs
> -----
> 
>   akonadi_next/kreparentingproxymodel.cpp 411e3fc 
>   akregator/src/importfeedlistcommand.cpp 117237b 
>   akregator/src/subscriptionlistview.cpp de41757 
>   calendarsupport/next/incidencechanger2.cpp dd9fbd1 
>   kaddressbook/printing/mikesstyle.cpp 1ac65cf 
>   kalarm/lib/spinbox.cpp ab2e9ec 
>   kjots/kjotsbrowser.cpp 3970cfc 
>   kleopatra/smartcard/readerstatus.cpp 3b9c282 
>   kleopatra/utils/gnupg-helper.cpp 02518db 
>   kmail/undostack.cpp d981769 
>   knode/foldertreewidget.cpp c0d1a92 
>   plugins/ktexteditor/ktexteditorkabcbridge.cpp 050d478 
> 
> Diff: http://git.reviewboard.kde.org/r/105149/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Jaime Torres Amate
> 
>

_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/