Thanks Attila, I have modified the patch
webrev : http://cr.openjdk.java.net/~pmuthuswamy/8191301/webrev.03/
Thanks,
Priya
On 12/13/2017 12:57 AM, Attila Szegedi wrote:
This is generally acceptable, and while I really don’t want to be a
pain, I’ll point out few minor things still, because I really want
this code to be the best it can be :-)
- in Nashorn code (and OpenJDK in general), we format “else” and “else
if” to be on the same line as the closing brace of the previous block,
so :
if (…) {
…
} else if (…) {
…
} else {
…
}
- in Nashorn, we declare all method parameters to be final.
- We strongly prefer final local variables with single-assignment
whenever possible, so instead of early-initializing “expression" to
null in getJavaImporter and then overwriting it, the preferred style
would be:
final NativeJavaImporter expression;
if (self instanceof NativeJavaImporter) {
expression = (NativeJavaImporter)self;
} else if (self instanceof ScriptObject) {
expression = getJavaImporterInScope((ScriptObject)self);
} else {
expression = null;
}
return expression;
The compiler will prove that a final variable is assigned exactly once
on each branch. This code is fairly simple so it might not matter
much, but it’s good to adopt this style of coding as it lets the
compiler help you ensure you didn’t forget to initialize the variable
on any path. Sometimes the early-initialization value, in this case
null, wouldn’t be valid in all cases and by providing it early, you
paper over all the missing cases and the compiler can’t tell if you
overlooked something. On the other hand, if you declare the variable
as final, it’ll report a compilation error if the variable is not
initialized on every possible path.
Alternatively, you could even do away with expression completely, and
just use early return statements like this:
if (self instanceof NativeJavaImporter) {
return (NativeJavaImporter)self;
}
if (self instanceof ScriptObject) {
return getJavaImporterInScope((ScriptObject)self);
}
return null;
but I don’t consider that to be more valid than the above version with
"final NativeJavaImporter expression”; they’re pretty much equivalent,
albeit this last one is slightly shorter.
Attila.
On 2017. Dec 12., at 18:31, Priya Lakshmi Muthuswamy
<priya.lakshmi.muthusw...@oracle.com
<mailto:priya.lakshmi.muthusw...@oracle.com>> wrote:
Hi Attila,
I have modified the patch with your suggestions.
webrev : http://cr.openjdk.java.net/~pmuthuswamy/8191301/webrev.02/
<http://cr.openjdk.java.net/%7Epmuthuswamy/8191301/webrev.02/>
Thanks,
Priya
On 12/12/2017 8:49 PM, Attila Szegedi wrote:
Hi Priya,
This indeed looks much better, although I do have some remarks with
regard to the the style of the code. Specifically, repetitions of
identical code, as well as assignments inside predicates.
There are several cases of code that is repeating:
First is:
((NativeJavaImporter)...).createProperty(JSType.toString(name))
Which occurs twice. You can avoid this by creating a “private static
NativeJavaImporter getJavaImporter(Object self)” method that either
returns self or does the lookup in scope, finally throws the type
error if it found nothing. Then __noSuchProperty__ can be simply
written as:
return getJavaImporter(self).createProperty(JSType.toString(name));
You have a similar duplication of ((WithObject)obj).getExpression()
in getJavaImporterInScope that you should avoid with an assignment
to a (final) local variable.
Also note that this method could return NativeJavaImporter as it
already tests the expression for being an instanceof
NativeJavaImporter. Basically, push the (NativeJavaImporter) cast
down into getJavaImporterInScope; it’ll look nicer from the point of
view of types than if you have to cast its return value in the caller.
Assignment in if: that’s discouraged because visually it’s easy to
overlook or mistake for a comparison check at a glance. Instead of
ScriptObject expression;
if (self instanceof ScriptObject && (expression =
getJavaImporterInScope((ScriptObject)self))!=null) {
return
((NativeJavaImporter)expression).createProperty(JSType.toString(name));
}
use
if (self instanceof ScriptObject) {
final NativeJavaExporter expression =
getJavaImporterInScope((ScriptObject)self);
If (expression != null) {
return ...
}
}
Attila.
On Dec 12, 2017, at 1:32 PM, Priya Lakshmi Muthuswamy
<priya.lakshmi.muthusw...@oracle.com
<mailto:priya.lakshmi.muthusw...@oracle.com>> wrote:
Hi,
Kindly review. I have modified the fix to work with multiple with
scopes.
webrev : http://cr.openjdk.java.net/~pmuthuswamy/8191301/webrev.01/
<http://cr.openjdk.java.net/%7Epmuthuswamy/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
<mailto: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/
<http://cr.openjdk.java.net/%7Epmuthuswamy/8191301/webrev.00/>
Thanks,
Priya