I updated PR #345 as per your comments (I believe). Let me know if it looks okay. I did require changing one swing test that was relying on the old toString behavior. It would be nice if Andres could comment if such a change could impact Griffon.
On Sat, Jun 18, 2016 at 6:04 PM, Jochen Theodorou <[email protected]> wrote: > 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 > >
