+1 Hannes
> Am 14.11.2016 um 04:09 schrieb Sundararajan Athijegannathan > <sundararajan.athijegannat...@oracle.com>: > > Looks good > > PS. LiteralNode in catch should be an array literal, right? i.e., false > or 3.14 or "hello" won't be accepted as catch param. Perhaps worth > adding a check (where catch param expression is checked). But, no biggie. > > -Sundar > > > On 11/12/2016 12:17 AM, Srinivas Dama wrote: >> 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 >
