<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/124369/">https://git.reviewboard.kde.org/r/124369/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On Lipiec 24th, 2015, 11:15 rano CEST, <b>Milian Wolff</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">the dialog UI needs to be revamped, the functionality and code is nice though - good work!</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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?</p></pre>
</blockquote>
<p>On Lipiec 24th, 2015, 2:06 po południu CEST, <b>Maciej Poleski</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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)?</p></pre>
</blockquote>
<p>On Lipiec 24th, 2015, 7:05 po południu CEST, <b>Maciej Poleski</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">There are two more reviews waiting for attention:
- https://git.reviewboard.kde.org/r/124300/
- https://git.reviewboard.kde.org/r/124285/</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">just sayin... :)</p></pre>
</blockquote>
<p>On Lipiec 25th, 2015, 3:25 po południu CEST, <b>Milian Wolff</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%"><span style="color: #408080; font-style: italic">// file 1:</span>
<span style="color: #008000; font-weight: bold">struct</span> foo {
<span style="color: #B00040">int</span> bar;
};
<span style="color: #408080; font-style: italic">// file 2:</span>
<span style="color: #BC7A00">#include "file1.h"</span>
{
foo f;
f.bar <span style="color: #666666">=</span> <span style="color: #666666">1</span>; <span style="color: #408080; font-style: italic">// translate that to f.setBar(1);</span>
}
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">? This is going to be a hard problem. I'd be <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">very</em> 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?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p></pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">External uses are already handled (everywhere in the world). For example this:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%"> auto nonAssignDeclRefExpr =
declRefExpr(expr().bind("DeclRefExpr"),
unless(anyOf(
hasParent(
binaryOperator(hasOperatorName("="),
hasLHS(equalsBoundNode("DeclRefExpr")))),
hasParent(
operatorCallExpr(hasOverloadedOperatorName("="),
argumentCountIs(2),
hasArgument(0, equalsBoundNode("DeclRefExpr")))))));
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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 <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">f.bar = 1</code> -> <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">f.setBar(1)</code> transformation).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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 <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">AccessSpecDecl</code>s in given record decl, collect them, find last <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">AccessSpecDecl</code> specifying access level i'm interested in, get the next (<code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">AccessSpecDecl</code>, but may not exist - separate handling), get its location (start), insert code <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">here</em>. Of course such access specifier may not even exist. In such a case i must select some place using different approach and generate required <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">AccessSpecDecl</code> (remembering about putting second <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">AccessSpecDecl</code> after generated code not to change access level of other members). Also changing access to selected field is quite strange.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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))</p></pre>
<br />
<p>- Maciej</p>
<br />
<p>On Lipiec 23rd, 2015, 4:07 po południu CEST, Maciej Poleski wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KDevelop.</div>
<div>By Maciej Poleski.</div>
<p style="color: grey;"><i>Updated Lip 23, 2015, 4:07 po południu</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdev-clang
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Encapsulate field refactoring</p>
<ul style="padding: 0;text-rendering: inherit;margin: 0 0 0 1em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Displays dialog</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Transforms "use" site - replaces direct uses of encapsulated variable by calls to getter and setter method/function.</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Generates getter and setter implementation</li>
</ul>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Attached image presents GUI with its default content (prefilled by heuristics on DeclaratorDecl AST node).</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Manual testing, unit testing</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>refactoring/CMakeLists.txt <span style="color: grey">(PRE-CREATION)</span></li>
<li>refactoring/encapsulatefielddialog.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>refactoring/encapsulatefielddialog.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>refactoring/encapsulatefielddialog.ui <span style="color: grey">(PRE-CREATION)</span></li>
<li>refactoring/encapsulatefieldrefactoring.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>refactoring/encapsulatefieldrefactoring.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>refactoring/encapsulatefieldrefactoring_changepack.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>refactoring/encapsulatefieldrefactoring_changepack.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>refactoring/redeclarationchain.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>refactoring/refactoringmanager.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>refactoring/tudecldispatcher.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>refactoring/tudecldispatcher.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>refactoring/utils.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>refactoring/utils.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>tests/CMakeLists.txt <span style="color: grey">(20d17efae9a2a77cd7ef76bf7484ccfcf12e4cd8)</span></li>
<li>tests/test_encapsulatefield.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>tests/test_encapsulatefield.cpp <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/124369/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">File Attachments </h1>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2015/07/15/ea15c55b-cc4d-4dfa-ac34-8c568a93701d__snapshot3.png">Encapsulate field dialog</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>