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

[Milian Wolff]

> On Jan. 10, 2012, 1:58 p.m., Milian Wolff wrote:
> > languages/cpp/codegen/generateaccessorsdialog.h, line 29
> > <http://git.reviewboard.kde.org/r/103613/diff/2/?file=46366#file46366line29>
> >
> >     please change the coding style here to be:
> >     
> >     funcname( ... );
> >     
> >     not
> >     
> >     funcname ( ... );
> >     
> >     do this everywhere please
> 
> Jonas Jacobi wrote:
>     this is the style created by the format sources configuration for that directory, shouldn't that file then be updated to the correct format?

yes.


> On Jan. 10, 2012, 1:58 p.m., Milian Wolff wrote:
> > languages/cpp/codegen/generateaccessorsdialog.h, line 58
> > <http://git.reviewboard.kde.org/r/103613/diff/2/?file=46366#file46366line58>
> >
> >     missing const& for identifier, qlist and QString
> >     
> >     also: passing the duchain lock looks very odd
> 
> Jonas Jacobi wrote:
>     - isn't const & for qlist/qstring kind of redundant, because they are implicitly shared anyway?
>     - instead of passing the lock i could, of course, unlock it before the call and create a separate lock in the method. do you prefer that?

- no, because const& is still faster as you don't have any overhead from the implicit sharing then
- regarding lock, see my comment further below in the implementation of that function. short form: try to do it asynchronously.


> On Jan. 10, 2012, 1:58 p.m., Milian Wolff wrote:
> > languages/cpp/codegen/generateaccessorsdialog.h, line 62
> > <http://git.reviewboard.kde.org/r/103613/diff/2/?file=46366#file46366line62>
> >
> >     on_foo_clicked() is not very Qt-like, call it:
> >     
> >     selectAllClicked()
> >     
> >     same for the functions below
> 
> Jonas Jacobi wrote:
>     the naming is for the qt auto signal slot connections, so i don't have to do them manually

qt auto signal slot connections? what is that? can you please show me a link? I still don't like the names though.


> On Jan. 10, 2012, 1:58 p.m., Milian Wolff wrote:
> > languages/cpp/codegen/generateaccessorsdialog.cpp, line 369
> > <http://git.reviewboard.kde.org/r/103613/diff/2/?file=46367#file46367line369>
> >
> >     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
> 
> Jonas Jacobi wrote:
>     I don't use isPublicBaseClass, because i don't have the actual QObject type, i search the hierarchy only by name.
>     If you could tell me, how i can get the type, or at least, where to look for it, i'll gladly change the code.

see e.g. ExpressionVisitor::qObjectPtrType() - please move this code and the related other method(s) to cppduchain/typeutils for reuse


- Milian


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


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/2dcba96d/attachment.html>