RFR: 8156615:Catch parameter can be a BindingPattern in ES6 mode

Attila Szegedi szegedia at gmail.com
Thu Nov 10 12:07:32 UTC 2016


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.dama at 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:szegedia at 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.athijegannathan at 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
>> 
> 



More information about the nashorn-dev mailing list