<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/119526/">https://git.reviewboard.kde.org/r/119526/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 29th, 2014, 5:11 a.m. UTC, <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;">I would prefer to look into this issue deeper, adding a hash of visited cursors isn't really an acceptable solution IMO.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Also not sure what is meant by "Apparently just 'recursing' vs. 'visiting children explicitly' results in a different AST traversal"</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">There should be no difference between clang_visitChildren on a cursor vs returning CXChildVisitRecurse. If we are doing both that is a problem, but that doesn't seem to be the case (IIRC we don't ever use CXChildVisitRecurse). On the other hand, returning CXChildVisitContinue means no further recursion is desired (ie, skip the children of this cursor).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This looks like a defect in the clang AST. Looking at it with clang-parser I see no reason for the StructDecl to be a child of the variable declaration. A workaround is still needed, but it should target this pattern without hashing every cursor in the TU, which would really be a last resort.</p></pre>
</blockquote>
<p>On July 29th, 2014, 5:48 a.m. UTC, <b>Kevin Funk</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 should be no difference between clang_visitChildren on a cursor vs returning CXChildVisitRecurse. If we are doing both that is a problem, but that doesn't seem to be the case (IIRC we don't ever use CXChildVisitRecurse). On the other hand, returning CXChildVisitContinue means no further recursion is desired (ie, skip the children of this cursor)."</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">=> Damn, I think that's indeed the case. I was playing around with DebugVisitor, removed the call to <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">clang_visitChildren</code> but forgot to make it return <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">CXChildVisit_Recurse</code> instead. So, indeed, even with <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">CXChildVisit_Recurse</code> the traversal is the same.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The issue still remains. We're visiting some cursors multiple times. I also don't really see why the StructDecl needs to be a child of the VarDecl. Maybe worth asking on the mailing list -- I'll do that.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I think it's still sane to have this committed for now (with a big-fat TODO), otherwise we and up duplicating decls and types in the DUChain.</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;">We don't need to pessimize every single cursor to introduce a hack for this case. Maybe try specializing dispatchCursor<CXCursor_StructDecl> and skipping it if the parent is a VarDecl (wrapped in the big-fat HACK/TODO markers of course). If needed you can even pass the parent in to dispatchCursor and update the switch.</p></pre>
<br />
<p>- Olivier Jean de</p>
<br />
<p>On July 28th, 2014, 8:25 p.m. UTC, Kevin Funk 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 Kevin Funk.</div>
<p style="color: grey;"><i>Updated July 28, 2014, 8:25 p.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;">Ensure we're not visiting cursors twice</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;">Please check, maybe I'm missing something.</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/tuduchain.h <span style="color: grey">(00ee23ba0da851456fd2a2641689514d25240389)</span></li>
<li>duchain/tuduchain.cpp <span style="color: grey">(a9e249e16a1d82c95c360b40575568abc1311444)</span></li>
<li>tests/test_duchain.h <span style="color: grey">(37ce20f398ea73d2a36d753a6421bb3015898f4d)</span></li>
<li>tests/test_duchain.cpp <span style="color: grey">(89f5005149421b8b3ef1a9def3d9e6222e3d6b0a)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/119526/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>