groovy-users mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jochen Theodorou <blackd...@gmx.org>
Subject Re: ShortTypeHandling castToString
Date Sat, 18 Jun 2016 08:04:18 GMT
On 15.06.2016 14:36, Winnebeck, Jason wrote:
> OK, I see the PR. I would be interested to hear Jochen's comments because the way the
class is documented it looks like that class is meant to be called in situations with constrained
conditions.

yes

> I mean, doesn't the normal String cast in Groovy go through DefaultGroovyMethods.asType?

no. asType is only for calling asType directly, or using the casting 
"as". A normal cast is done by the means of 
ScriptBytecodeAdapter#castToType and friends. The idea with 
ShortTypeHandling was to have a class with no dynamic method calls and 
small methods for the easy cases, with reuse in the static compiler 
maybe. The default for a cast using "as", is the normal cast.

> I see in that method there is a specialization when casting to String that calls InvokerHelper.toString,
which has a lot of logic in it. For your issue GROOVY-7853 around primitive arrays I see that
asType goes to InvokerHelper.toString, which calls InvokerHelper.format, which sees its class
"isArray()" is true, then it has a specialization for char[] but if it's not char[] it casts
the array to a collection and invokes Invoker.format on that -- which seems to go through
a lot of effort casting everything in there recursively to a String when everything is guaranteed
to be a primitive at that point.
>
> In that sense, even the fix provided in PR 345 doesn't match the standard Groovy cast
to String operator, which recursively formats the elements in the array, instead you just
use toString.
>
> But I also wonder if the real bug is that the compiler generates ShortTypeHandling calls
when it should be reverting to the full asType instead.

there is also the problem with a dynamically added toString() method.
Actually GROOVY-7853 makes me think that this method in 
ShortTypeHandling should not be called at all and that the castToType 
route should be taken instead.... and ensured we will pick up the 
dynamic toString method there as well.

bye Jochen



Mime
View raw message