<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/120086/">https://git.reviewboard.kde.org/r/120086/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On September 7th, 2014, 1:10 p.m. MSK, <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;">Sergey, can you explain why this is an issue for out-of-line methods? Is the Identifier for e.g. "foo::bar" not "bar", but something else? Or is the range start column pointing to "foo" instead of "bar" and then adding the length of the Identifier to it makes it wrong?</p></pre>
</blockquote>
<p>On September 7th, 2014, 1:13 p.m. MSK, <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;">note: when it's the latter, e.g. if the scope is wrongly taken into account, is there a way to change the clang_Cursor_getSpellingNameRange call to start at the "bar"?</p></pre>
</blockquote>
<p>On September 7th, 2014, 4:18 p.m. MSK, <b>Sergey Kalinichev</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;">Example:<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
class foo {<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
void bar();<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
};<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
// Here is the id.toString() == "foo::bar", range start column at bar<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
void foo::bar(){<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
}</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So, if I understand you right the id.toString() should be just "bar"? And it should be fixed in some other place then?</p></pre>
</blockquote>
<p>On September 7th, 2014, 4:35 p.m. MSK, <b>Olivier Jean de Gaalon</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;">See TUDUChain::makeId, where kdev-clang special cases CXXMethod identifiers. I was not aware this is done. Oldcpp did the same thing, but I'd like to find a better solution; maybe creating helper contexts with the class identifier.</p></pre>
</blockquote>
<p>On September 7th, 2014, 4:42 p.m. MSK, <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 Identifier should be "foo", the QualifiedIdentifier otoh should be "foo::bar".</p></pre>
</blockquote>
<p>On September 7th, 2014, 6:30 p.m. MSK, <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;">It might be required to use a QualifiedIdentifier in more places instead of Identifier. Then we can detect this here, and only extend the range of the use by the last element of the QualifiedIdentifier. The best solution of course would be to fix this upstream in clang... Which was done already (afaik?) :) So maybe we can do a costly workaround and ifdef that based on the clang version?</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;">Hmm, I'm just wondering why is this CXXMethod identifiers workaround in TUDUChain::makeId needed at all?<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
I've just tested it without it. Everything works fine with out-of-line methods: i.e. code-navigation, code highlighting, uses count, rename features e.t.c.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
So, it's not clear for me why is it needed at all, except "that's how oldcpp does it". Maybe it just workarounds it's own bugs that way?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Also note, that currently rename capabilities don't work for out-of-line methods (actually this patch doesn't fix it too). But removing that CXXMethod identifiers workaround fixes it.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So, I'm not sure if we should do it like the oldcpp does it. Maybe it'd be better to just just remove it?</p></pre>
<br />
<p>- Sergey</p>
<br />
<p>On September 7th, 2014, 10:20 a.m. MSK, Sergey Kalinichev 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 Sergey Kalinichev.</div>
<p style="color: grey;"><i>Updated Sept. 7, 2014, 10:20 a.m.</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;">This was accidentally broken by:<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
f17866db1bc24c198e87c028065796959f6dd135<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
Author: Kevin Funk <a href="mailto:kfunk@kde.org" style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">kfunk@kde.org</a><br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
Date: Mon Aug 4 16:53:06 2014 +0200</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%">Also fix ranges <span style="color: #008000; font-weight: bold">for</span> operator functions
</pre></div>
</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;">All tests pass.</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>duchain/clanghelpers.cpp <span style="color: grey">(fb69a66)</span></li>
<li>tests/files/functiondefinitiondeclarations.cpp <span style="color: grey">(fe4050e)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/120086/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>