blackdrag commented on pull request #1309: URL: https://github.com/apache/groovy/pull/1309#issuecomment-662355159
@paulk-asert @danielsun1106 now you guys forced me to look into this in much more detail and thought. The current implementation in this PR will build the string potentially only once and then this will be the final result. The part with clone I did originally not even see (I think I looked at it before you did that change), also does not improve performance. But you need it to ensure the output string is always the same. But I think this is all thought a bit to hasty/short, because you have no control over the subclasses of GString, which might be another class than GStringimpl: * breaking change: subclass may override getValues and expect changes from there taken into account for the final string. Since you are now using this.value, this cannot happen anymore * breaking change: you now assume all subclasses of GString will be GStringImpl by imposing the restriction on getStrings. But nothing can ensure this right now * breaking change: classes subclassing GStringImpl and overriding getStrings not returning the same strings will be ignored in GString * breaking change: calling getValues and then changing an element of the array no longer takes affect * breaking change: calling getStrings and then changing an element of the array no longer takes affect, if the implementing class is GStringImpl If we really wanted to go in this direction, then I would do the following: * merge GString and GStringImpl, since the implementation split with unchecked restrictions makes no sense for more than one possible subclass * make getValues and getStrings final or the class itself, since it makes no sense for classes to change that anymore * add a method to change the behaviour of GString between calculating once and every time. (property could be used to control the default) But actually I am unsure about this last point... because if you change the behaviour it works fine with mutable classes, for which this PR already caters. So it is values/strings set from outside that really matter here, and the PR does actually not allow them being set, because of the methods returning a cloned array. Which means for me, that the current version of this PR is quite a stripped down version of what it was before and more a structured string, than what it was before. Not that this is bad in itself, it just has other use cases than GString. If all the old use cases of GString are important to us, we cannot do this here like this. If not, well, then we have a major breaking change. We could of course also do this slightly different. Let's say: * keep getValues and getStrings return the cloned array * add mutator methods for values and strings which will reset the output string This is still a breaking change of course and especially the part with changing array elements of getValues being ignored is not nice, since it will be a silent bug. Instead I would think of these methods returning a list instead and make that immutable, which allows for better error messages. ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected]
