<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/118483/">https://git.reviewboard.kde.org/r/118483/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On June 2nd, 2014, 10:43 p.m. UTC, <b>Sven Brauch</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;">Hmm. You can turn off declarations from being in the symbol table (setInSymbolTable(false)). I'm not sure if this is the right solution here because I don't know by heart if class members should be in the symbol table (I'll have to look that up) but I guess you could try it.</pre>
</blockquote>
<p>On June 3rd, 2014, 9 a.m. UTC, <b>Denis Steckelmacher</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;">In C++, class members are not in the symbol table, but methods are. I think that I will call setInSymbolTable(false) on array keys, because they will most of the time belong to an anonymous class (an object literal), and the same name may be used by many different objects (think of "prototype" that is set for many functions).</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;">setInSymbolTable works great, and I rejected this patch because I've discovered that it was buggy:
function foo() { return true; }
var a = {key: foo()};
In this snippet, "foo" was not found in the context of a. Without this patch, this works well. So, using findLocalDeclarations is not the solution. Thanks for the advice!</pre>
<br />
<p>- Denis</p>
<br />
<p>On June 3rd, 2014, 9:04 a.m. UTC, Denis Steckelmacher wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDevelop.</div>
<div>By Denis Steckelmacher.</div>
<p style="color: grey;"><i>Updated June 3, 2014, 9:04 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdev-qmljs
</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;">This is a workaround for a behavior that I cannot completely explain. Here is a snippet of Javascript code that exhibits the behavior:
----
var a = {key: "value"};
key = 2;
----
Without my patch, "key = 2" is considered to be an use of a.key. However, the declaration of "key" is in a context that spans the space inside the brackets. Some debugging has even shown that "key" is not listed in DUContext::getDeclarations(), called on the context in which "key = 2" appears (the top-level context):
----
looking for "key":
<struct> a
-> found "string key"
----
By looking at the code and running GDB, I've seen that when the context in which a declaration is being looked for is a TopDUContext, then the symbol table is used instead of the normal code path. The problem is that "key" appears in the symbol table (I think this is expected), but I don't want it to be "visible" from the top-level context.
This patch is therefore a mean to avoid using TopDUContext::getDeclarations. First, getLocalDeclarations is tried (this finds declaration in the current context and its parents, but not its imported parent contexts, if I've correctly understood how the thing works), then, and only if the context has imported parent contexts, getDeclarations is used.
This is quite a dirty solution. Is there a mean to prevent TopDUContext from short-circuiting the normal declaration filtering process and from returning declarations that should normally not be visible?</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;">The whole testsuite passes, but the patch seems a little fragile: the top-level context may also have imported parent contexts (if there are import statements, or in the future when Javascript require() statements will be supported), and so the second if may well be entered even if context is a TopDUContext. The patch therefore does not fix the bug in all cases.</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/helper.cpp <span style="color: grey">(27f5dd4)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/118483/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>