Excellent! +1
Attila. > On 2017. Dec 13., at 4:42, Priya Lakshmi Muthuswamy > <priya.lakshmi.muthusw...@oracle.com> wrote: > > Thanks Attila, I have modified the patch > > webrev : http://cr.openjdk.java.net/~pmuthuswamy/8191301/webrev.03/ > <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 >>>>>>>> <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 >>>>>>>> >>>>>>>> >>> >> >
