Review Request 124369: Encapsulate field refactoring
Maciej Poleski
d82ks8djf82msd83hf8sc02lqb5gh5 at gmail.com
Sat Jul 25 15:25:33 UTC 2015
> On Lip 24, 2015, 11:15 rano, Milian Wolff wrote:
> > the dialog UI needs to be revamped, the functionality and code is nice though - good work!
> >
> > a) the line edit for the field to encapsulate, why is it a line edit? or is it read only? what happens if the user changes the contents?
> > b) the dialog structure is really awkward, I have to say. please use a form layout with labels left and widgets right. Insteaqd of the radio boxes, use combo boxes, and don't use a sentence structure to layout the widgets, that will break horribly once the dialog is translated (think about RTL languages!). And maybe, it's best to let the user configure the signature in pure line edits? Adding combo boxes for noexcept etc. pp. will never scale. Or maybe even a text edit so the bodies can be edited directly as well? My current approach would be something like this: http://i.imgur.com/wWHOIHd.png the sources you can find at https://paste.kde.org/pdrz1iqo6 . I'm only unsure whether we can easily get the name of the setter argument to create the body. And should we also allow configuration about inlining the bodies? Probably a good idea for the future?
>
> Maciej Poleski wrote:
> ad a) line edit is read only. it is information for user which field is going to be encapsulated. user cannot change contents.
> ad b) i'm very bad GUI designer, that's fact... radio boxes were used because need one click instead of two in combo boxes. scaling is an issue, i like idea to provide text edits with bodies (prefilled with default generated body). Currently implementation is placed in (all) RecordDecl (class or structure). I'm still thinking how to find appropriate place in appropriate implementation file. I can get "path" of record decls up to TU record decl (root), but then how to find the same scope (place) in another TU (and prove that this TU will be available available after compilation to all uses)?
>
> Maciej Poleski wrote:
> There are two more reviews waiting for attention:
> - https://git.reviewboard.kde.org/r/124300/
> - https://git.reviewboard.kde.org/r/124285/
>
> just sayin... :)
>
> Milian Wolff wrote:
> Would be cool if you could take some ideas from my GUI approach and mix and match it with your approach to get something useable.
>
> Regarding your problem to find the implementation file, there are helper functions to figure out whether you are in a header or not. If you are not, then just add it to the current file. Otherwise, use the buddy feature to find the corresponding implementation file, so just reuse that code please.
>
> Then, I don't know what you mean with the "record decls" stuff at the end - do you mean external users of the field you just wrapped in a getter/setter? Like:
>
> // file 1:
> struct foo {
> int bar;
> };
> // file 2:
> #include "file1.h"
> {
> foo f;
> f.bar = 1; // translate that to f.setBar(1);
> }
>
> ? This is going to be a hard problem. I'd be _very_ find with tackling the "simple" issue first of creating some code. Then, maybe, we should concentrate first on more local changes to get some useable stuff, like I mentioned before. What do you think? Tackling these hard problems we can still do later, no?
>
> Sorry for lagging behind, my day job and now Akademy often keeps me distracted. Please continue to ping me about things I'm missing or not responding to quickly enough.
I made assumption stronger than ODR rule (http://en.cppreference.com/w/cpp/language/definition). I require only one literal definition of record decl (struct or class) in the whole world. Such definition can be in some header, included from other files. But it cannot be "copy-pasted" to other files (which could still comply to ODR in some cases). I don't know about codes which do copy-paste definitions of classes (instead of including appropriate header).
External uses are already handled (everywhere in the world). For example this:
```
auto nonAssignDeclRefExpr =
declRefExpr(expr().bind("DeclRefExpr"),
unless(anyOf(
hasParent(
binaryOperator(hasOperatorName("="),
hasLHS(equalsBoundNode("DeclRefExpr")))),
hasParent(
operatorCallExpr(hasOverloadedOperatorName("="),
argumentCountIs(2),
hasArgument(0, equalsBoundNode("DeclRefExpr")))))));
```
matches all uses of static field which are not on left-hand-side of assign (=) operator. These will be transformed to use generated getter instead. In fact what is hard is just the opposite: generating code (accessors and mutators). Clang can help me (a bit) looking for some particular patterns in code (not including reasoning if a given reference (DeclRefExpr) really refers to field i'm interested in encapsulating - this is the reason of my assumption i mentioned earlier). The above example (with struct foo and two files) is already translated as necessary (including `f.bar = 1` -> `f.setBar(1)` transformation).
I will definitely use buddy feature (i seen this somewhere in KDevPlatform interfaces, is this what You mean?). At least to optimize a few things.
In case of this particular refactoring the most difficult part was not even generating accessors code (it's still quite easy). Difficult is to select place in record decl (definition of class containing field we are encapsulating) for generated implementation. In short i'm looking for all `AccessSpecDecl`s in given record decl, collect them, find last `AccessSpecDecl` specifying access level i'm interested in, get the next (`AccessSpecDecl`, but may not exist - separate handling), get its location (start), insert code *here*. Of course such access specifier may not even exist. In such a case i must select some place using different approach and generate required `AccessSpecDecl` (remembering about putting second `AccessSpecDecl` after generated code not to change access level of other members). Also changing access to selected field is quite strange.
Try to imagine how these things are going to be even more difficult if we want to put implementation in separate place (which of course must be choosen appropriately) and leaving only declaration of accessors in record decl.
(Nonetheless I have some early ideas how this refactoring (move function definition) could be operating. i could for example try to remember path from given AST node to root of AST tree and try to walk this path from AST root in target TU creating nested scopes as necessary (but lot of cases to handle, some probably in general impossible))
- Maciej
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124369/#review82859
-----------------------------------------------------------
On Lip 23, 2015, 4:07 po południu, Maciej Poleski wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124369/
> -----------------------------------------------------------
>
> (Updated Lip 23, 2015, 4:07 po południu)
>
>
> Review request for KDevelop.
>
>
> Repository: kdev-clang
>
>
> Description
> -------
>
> Encapsulate field refactoring
>
> - Displays dialog
> - Transforms "use" site - replaces direct uses of encapsulated variable by calls to getter and setter method/function.
> - Generates getter and setter implementation
>
> Attached image presents GUI with its default content (prefilled by heuristics on DeclaratorDecl AST node).
>
>
> Diffs
> -----
>
> refactoring/CMakeLists.txt PRE-CREATION
> refactoring/encapsulatefielddialog.h PRE-CREATION
> refactoring/encapsulatefielddialog.cpp PRE-CREATION
> refactoring/encapsulatefielddialog.ui PRE-CREATION
> refactoring/encapsulatefieldrefactoring.h PRE-CREATION
> refactoring/encapsulatefieldrefactoring.cpp PRE-CREATION
> refactoring/encapsulatefieldrefactoring_changepack.h PRE-CREATION
> refactoring/encapsulatefieldrefactoring_changepack.cpp PRE-CREATION
> refactoring/redeclarationchain.cpp PRE-CREATION
> refactoring/refactoringmanager.cpp PRE-CREATION
> refactoring/tudecldispatcher.h PRE-CREATION
> refactoring/tudecldispatcher.cpp PRE-CREATION
> refactoring/utils.h PRE-CREATION
> refactoring/utils.cpp PRE-CREATION
> tests/CMakeLists.txt 20d17efae9a2a77cd7ef76bf7484ccfcf12e4cd8
> tests/test_encapsulatefield.h PRE-CREATION
> tests/test_encapsulatefield.cpp PRE-CREATION
>
> Diff: https://git.reviewboard.kde.org/r/124369/diff/
>
>
> Testing
> -------
>
> Manual testing, unit testing
>
>
> File Attachments
> ----------------
>
> Encapsulate field dialog
> https://git.reviewboard.kde.org/media/uploaded/files/2015/07/15/ea15c55b-cc4d-4dfa-ac34-8c568a93701d__snapshot3.png
>
>
> Thanks,
>
> Maciej Poleski
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20150725/0495ec2f/attachment.html>
More information about the KDevelop-devel
mailing list