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
