Review Request: Generate accessors (getters/setters) for C++

Milian Wolff mail at milianw.de
Tue Jan 10 13:58:36 UTC 2012 

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


phew huge changeset, huge review. please first work on these and try to go also through the files I did not review then update this request.

thanks


languages/cpp/codegen/generateaccessorsdialog.h
<http://git.reviewboard.kde.org/r/103613/#comment7988>

    this file is missing a copyright header with your name in it
    
    please use GPL v2+ or higher



languages/cpp/codegen/generateaccessorsdialog.h
<http://git.reviewboard.kde.org/r/103613/#comment7987>

    please change the coding style here to be:
    
    funcname( ... );
    
    not
    
    funcname ( ... );
    
    do this everywhere please



languages/cpp/codegen/generateaccessorsdialog.h
<http://git.reviewboard.kde.org/r/103613/#comment7990>

    please use /** ... */ instead of multiple lines with ///



languages/cpp/codegen/generateaccessorsdialog.h
<http://git.reviewboard.kde.org/r/103613/#comment7989>

    please trim trailing whitespace (in kate: f7 -> rtrim)



languages/cpp/codegen/generateaccessorsdialog.h
<http://git.reviewboard.kde.org/r/103613/#comment7991>

    if it does not exist yet in the cppduchain helpers, then move this function there



languages/cpp/codegen/generateaccessorsdialog.h
<http://git.reviewboard.kde.org/r/103613/#comment8000>

    missing const& for identifier, qlist and QString
    
    also: passing the duchain lock looks very odd



languages/cpp/codegen/generateaccessorsdialog.h
<http://git.reviewboard.kde.org/r/103613/#comment7999>

    call it stripPrefixesChanged() and const& the QString arg



languages/cpp/codegen/generateaccessorsdialog.h
<http://git.reviewboard.kde.org/r/103613/#comment7998>

    on_foo_clicked() is not very Qt-like, call it:
    
    selectAllClicked()
    
    same for the functions below



languages/cpp/codegen/generateaccessorsdialog.h
<http://git.reviewboard.kde.org/r/103613/#comment7997>

    I'd prefer the name m_usedAccesssPolicies, same for the values below



languages/cpp/codegen/generateaccessorsdialog.cpp
<http://git.reviewboard.kde.org/r/103613/#comment7992>

    again, missing copyright (see above)



languages/cpp/codegen/generateaccessorsdialog.cpp
<http://git.reviewboard.kde.org/r/103613/#comment7995>

    I'd like to see these headers cleaned up:
    
    <QStandardItemModel>
    
    group Qt includes, KDE includes, KDevplatform includes, ...



languages/cpp/codegen/generateaccessorsdialog.cpp
<http://git.reviewboard.kde.org/r/103613/#comment7993>

    wrong include style: <language/...>



languages/cpp/codegen/generateaccessorsdialog.cpp
<http://git.reviewboard.kde.org/r/103613/#comment7994>

    here: <interfaces/iplugin.h>



languages/cpp/codegen/generateaccessorsdialog.cpp
<http://git.reviewboard.kde.org/r/103613/#comment7996>

    here as well: please don't put a whitespace before the (
    
    also please move the initializer to its own line:
    
    class::ctor()
      : foo(bar)
      , asdf(yxcv)
      , ...
    {
    
    }



languages/cpp/codegen/generateaccessorsdialog.cpp
<http://git.reviewboard.kde.org/r/103613/#comment8001>

    please change the ui names, don't put the type (txt, chk, rdo, ...) into the name
    
    stripPrefixes alone is fine enough



languages/cpp/codegen/generateaccessorsdialog.cpp
<http://git.reviewboard.kde.org/r/103613/#comment8002>

    can be merged with the slot above, or just create a helper function that takes the Qt::Checked/Unchecked argument



languages/cpp/codegen/generateaccessorsdialog.cpp
<http://git.reviewboard.kde.org/r/103613/#comment8003>

    again, same as above - please share code



languages/cpp/codegen/generateaccessorsdialog.cpp
<http://git.reviewboard.kde.org/r/103613/#comment8004>

    this looks wrong, why don't you just use ClassDeclaration::isPublicBaseClass ?
    
    also, this should be done somewhere else already - just grep the code. for signal/slot support e.g. we also need to know whether we have a QObject or not. So please share code, i.e. use stuff from cppduchain helpers or push existing code there and then reuse it



languages/cpp/codegen/generateaccessorsdialog.cpp
<http://git.reviewboard.kde.org/r/103613/#comment8005>

    { on a newline please



languages/cpp/codegen/generateaccessorsdialog.cpp
<http://git.reviewboard.kde.org/r/103613/#comment8006>

    better connect to the signal and wait asyncronously for the update. when it's there create the separate definition. that also makes the odd passing of the lock go away



languages/cpp/codegen/generateaccessorsmodel.h
<http://git.reviewboard.kde.org/r/103613/#comment8007>

    again missing copyright
    
    furthermore there are *tons* of classes in this file - please put them into their own files, maybe in a generateaccessors subfolder



languages/cpp/codegen/generateaccessorsmodel.h
<http://git.reviewboard.kde.org/r/103613/#comment8009>

    please cleanup includes like above: same style, group same includes



languages/cpp/codegen/generateaccessorsmodel.h
<http://git.reviewboard.kde.org/r/103613/#comment8008>

    wrong include see above



languages/cpp/codegen/generateaccessorsmodel.h
<http://git.reviewboard.kde.org/r/103613/#comment8010>

    const& QString, also in the funcs below



languages/cpp/codegen/generateaccessorsmodel.h
<http://git.reviewboard.kde.org/r/103613/#comment8011>

    const& both args



languages/cpp/codegen/generateaccessorsmodel.h
<http://git.reviewboard.kde.org/r/103613/#comment8012>

    personally I'd prefer m_ -style naming



languages/cpp/codegen/generateaccessorsmodel.h
<http://git.reviewboard.kde.org/r/103613/#comment8013>

    const&



languages/cpp/codegen/generateaccessorsmodel.h
<http://git.reviewboard.kde.org/r/103613/#comment8014>

    again I'd prefer m_



languages/cpp/codegen/generateaccessorsmodel.h
<http://git.reviewboard.kde.org/r/103613/#comment8015>

    trim whitespace



languages/cpp/codegen/generateaccessorsmodel.h
<http://git.reviewboard.kde.org/r/103613/#comment8016>

    const&



languages/cpp/codegen/generateaccessorsmodel.h
<http://git.reviewboard.kde.org/r/103613/#comment8017>

    don't inline this



languages/cpp/codegen/generateaccessorsmodel.h
<http://git.reviewboard.kde.org/r/103613/#comment8018>

    I'd prefer m_



languages/cpp/codegen/generateaccessorsmodel.cpp
<http://git.reviewboard.kde.org/r/103613/#comment8019>

    missing copyright, please cleanup includes


- Milian Wolff


On Jan. 10, 2012, 12:23 p.m., Jonas Jacobi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103613/
> -----------------------------------------------------------
> 
> (Updated Jan. 10, 2012, 12:23 p.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Description
> -------
> 
> Generate accessors (getters/setters) for C++.
> 
> This patch adds the possibility to create accessor methods for class member variables via the gui.
> The dialog offers the following features:
> - shows all members of a selected class in a tree view (with getters/setters as children), if it was opened on a class member that one is pre-selected
> - only shows setters if applicable (non-const member)
> - accessor method definitions can be created inline or separate (in the fitting source file, if applicable)
> - configurable automatic creation of method names with manual override for each accessor method
> - access policy (visibility) setting global and individualy for each accessor method
> - configurable default setter parameter name with manual override for each setter (void setFoo(int myParamterName))
> - selectable getter return type (e.g. Type, Type&, const Type& for a member of type Type)
> - create setters as slots for QObject subclasses
> 
> The code is in a usable state and i would like to get some feedback on the dialog, especially on usability improvements.
> 
> Regards,
> Jonas
> 
> 
> Diffs
> -----
> 
>   languages/cpp/CMakeLists.txt a1fb97c 
>   languages/cpp/codegen/generateaccessorsdialog.h PRE-CREATION 
>   languages/cpp/codegen/generateaccessorsdialog.cpp PRE-CREATION 
>   languages/cpp/codegen/generateaccessorsmodel.h PRE-CREATION 
>   languages/cpp/codegen/generateaccessorsmodel.cpp PRE-CREATION 
>   languages/cpp/codegen/simplerefactoring.h b2187c3 
>   languages/cpp/codegen/simplerefactoring.cpp fd62ccf 
>   languages/cpp/codegen/ui/cppgenerateaccessors.ui PRE-CREATION 
>   languages/cpp/cppduchain/sourcemanipulation.h 6f8a79b 
>   languages/cpp/cppduchain/sourcemanipulation.cpp 3f40eb2 
>   languages/cpp/tests/CMakeLists.txt 40d07b0 
>   pics/mini/private_slot.png PRE-CREATION 
>   pics/mini/protected_slot.png PRE-CREATION 
>   pics/mini/public_slot.png PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/103613/diff/diff
> 
> 
> Testing
> -------
> 
> 
> Screenshots
> -----------
> 
> 
>   http://git.reviewboard.kde.org/r/103613/s/412/
> 
>   http://git.reviewboard.kde.org/r/103613/s/413/
> 
> 
> Thanks,
> 
> Jonas Jacobi
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20120110/0f6023ca/attachment.html>

More information about the KDevelop-devel mailing list