Hi, Please review webrev: http://cr.openjdk.java.net/~sdama/8156615/webrev.03/ Bug : https://bugs.openjdk.java.net/browse/JDK-8156615
@Attila, Thank you for the comments. Regards, Srinivas -----Original Message----- From: Attila Szegedi [mailto:szege...@gmail.com] Sent: Thursday, November 10, 2016 5:38 PM To: Nashorn-dev Subject: Re: RFR: 8156615:Catch parameter can be a BindingPattern in ES6 mode I’m still not happy with this, although we’re getting there :-) - you changed getException() to return Expression, even though Sundar asked you to add a different method. I don’t like the fact that getException() returns Expression, since it can’t be an arbitrary expression; it’s limited to identifier and binding pattern, yet you have unchecked casts on its return value on invocation everywhere now. Maybe keep your “Expression getException()” change, but add “IdentNode getExceptionIdentifier() { return (IdentNode)exception; }” and have the callers use that now instead. That way, you encapsulate the cast and don’t force the users of the API to do it everywhere, and it’s easier to track with call hierarchy in the IDEs which callers presume the exception is an identifier. - the JavaDoc for getException() and setException() should be updated to reflect the value can be a binding pattern. Please use the wording “binding pattern” instead of just “pattern” in the documentation (including the field declaration) to avoid ambiguity. - Maybe throw an IllegalArgumentException if either the constructor or setException receive something that isn’t either an ident node or a binding pattern. That way, you could enforce an invariant in the class that the types of the expression are only legitimate ones. I’m unsure if it’s easy to recognize the binding pattern or not (e.g. if it just looks like an object/array literal). Attila. > On 10 Nov 2016, at 12:46, Srinivas Dama <srinivas.d...@oracle.com> wrote: > > Hi, > > Please review > Bug: https://bugs.openjdk.java.net/browse/JDK-8156615 > webrev: http://cr.openjdk.java.net/~sdama/8156615/webrev.02/ > > Included all changes suggested with some modifications. > > Regards, > Srinivas > > -----Original Message----- > From: Attila Szegedi [mailto:szege...@gmail.com] > Sent: Tuesday, November 08, 2016 7:40 PM > To: Nashorn-dev > Subject: Re: RFR: 8156615:Catch parameter can be a BindingPattern in > ES6 mode > > What Sundar said. Additionally: > > + if (exception instanceof IdentNode) { > + this.exception = ((IdentNode) exception == null) ? null : > + ((IdentNode) exception).setIsInitializedHere(); > > First, casting an expression before a null comparison is silly :-). > Second, you don't even need to check for exception == null as "exception > instanceof IdentNode" implies that exception != null. So that line should > just be: > > + if (exception instanceof IdentNode) { > + this.exception = ((IdentNode) exception).setIsInitializedHere(); > > Attila. > >> On 08 Nov 2016, at 04:45, Sundararajan Athijegannathan >> <sundararajan.athijegannat...@oracle.com> wrote: >> >> You need to update Parser API implementation to handle catch >> parameter being expression - rather than an IdentNode. >> >> => you need to >> >> * add another getter in CatchNode to expose Expression >> >> * Use it in IRTranslator (of Parser API impl.) to translate catch >> param as expression >> >> * You need to check for expression param in codegen pipeline to throw >> "not yet implemented" error. >> >> -Sundar >> >> On 11/7/2016 9:40 PM, Srinivas Dama wrote: >>> Please review: >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8156615 >>> Webrev: http://cr.openjdk.java.net/~sdama/8156615/webrev.00/ >>> >>> Please note that, this contains only parsing support for catch parameter as >>> BindingPattern or BindingIdentifier. >>> >>> Regards, >>> Srinivas >> >
