+1, and I like the fact that the patch has become smaller.

Hannes


> Am 12.12.2017 um 13:32 schrieb Priya Lakshmi Muthuswamy 
> <priya.lakshmi.muthusw...@oracle.com>:
> 
> Hi,
> 
> Kindly review. I have modified the fix to work with multiple with scopes.
> 
> webrev : http://cr.openjdk.java.net/~pmuthuswamy/8191301/webrev.01/
> 
> Thanks,
> Priya
> On 12/5/2017 12:54 PM, Priya Lakshmi Muthuswamy wrote:
>> Hi Attila,
>> 
>> Thanks for review.
>> Yes when I checked with two with scopes as suggested(JavaImporter as outer), 
>> current fix doesn't work. I will work on that.
>> 
>> Thanks,
>> Priya
>> On 12/5/2017 12:12 PM, Attila Szegedi wrote:
>>> Hm… this seems to be an issue with shared scope calls; that’s why it’s 
>>> sensitive to the number of similar statements.
>>> 
>>> That said, here’s some thoughts:
>>> 
>>> 1. Instead of
>>> 
>>>     if (self instanceof ScriptObject && 
>>> ((ScriptObject)self).hasWithScope()) {
>>> 
>>> you should be able to just use
>>> 
>>>     if (self instanceof ScriptObject) {
>>> 
>>> As then you’ll evaluate getWithScopeObject and test it for being null 
>>> anyway. This way, you avoid walking the prototype chain twice.
>>> 
>>> 2. That said, I guess hasWithScope could be reimplemented simply as
>>> 
>>>     public boolean hasWithScope() {
>>>         return getWithScopeObject() != null;
>>>     }
>>> 
>>> as both have very similar code, so it’d reduce to it nicely. (I understand 
>>> that you haven’t changed that, but since you were in the vicinity of that 
>>> code, you might as wel do it… It’s also fine if you leave it alone as it 
>>> is.)
>>> 
>>> 3. One of the statements in the test is indented differently than the 
>>> others.
>>> 
>>> 4. What happens if there’s _two_ with scopes, and the JavaImporter is in 
>>> the outer one? Does this fix still work? E.g.:
>>> 
>>> var imports = new JavaImporter(java.lang);
>>> var dummy = { x: 42, y: 13 }
>>> with (imports) {
>>>      with (dummy) {
>>>          function func() {
>>>              System.out.println('a');
>>>             System.out.println('a');
>>>         System.out.println('a');
>>>              System.out.println('a');
>>>             System.out.println('a');
>>>         System.out.println('a');
>>>             System.out.println('a');
>>>          };
>>>          func();
>>>     }
>>> }
>>> 
>>> Attila.
>>> 
>>>> On Dec 5, 2017, at 7:13 AM, Priya Lakshmi Muthuswamy 
>>>> <priya.lakshmi.muthusw...@oracle.com> wrote:
>>>> 
>>>> Hi,
>>>> 
>>>> please review JDK-8191301 : JavaImporter fails to resolve imported 
>>>> elements within functions, that contain too many statements
>>>> 
>>>> JBS : https://bugs.openjdk.java.net/browse/JDK-8191301
>>>> webrev : http://cr.openjdk.java.net/~pmuthuswamy/8191301/webrev.00/
>>>> 
>>>> Thanks,
>>>> Priya
>>>> 
>>>> 
>> 
> 

Reply via email to