RFR: 8330462: StringIndexOutOfBoundException when typing anything into TextField [v9]

[Andy Goryachev]

On Sat, 20 Apr 2024 14:50:15 GMT, Oliver Kopp <duke at openjdk.org> wrote:

>> modules/javafx.graphics/src/main/java/com/sun/glass/ui/win/WinTextRangeProvider.java line 116:
>> 
>>> 114:             return fixedMaxEnd;
>>> 115:         }
>>> 116:     }
>> 
>> Frankly, I have hard time understanding this code (maybe a comment describing what the method does and why might help).
>> 
>> It looks to me that all we need to do is to guard against a very large maxLength (which for some reason called here 'requestedSteps' which does not seem right - should the last two arguments be swapped?)
>> 
>> 
>> public static int getEndIndex(int start, int length, int maxLength) {
>>   if(length > maxLength) {
>>     length = maxLength;
>>   }
>>   return start + length;
>> }
>> 
>> 
>> That is, I assume we don't have to worry about start + fixedLength overflowing, we just need to make sure we don't go beyond maxLength.  Or is my assumption wrong and start can be negative, or start+fixedLength might overflow?
>
> Smalltalk 1: I thought this was easier to understand than some byte code generation in the JVM. 😅  (For me being an outsider, JDK and JavaFX are "close" related somehow - sorry for that!!)
> 
> Smalltalk 2: Sometimes, the code for "Calculate a + b, but return c at most" is pretty hard to craft.
> 
> Smalltalk 3: The whole checks stem from possible "out of range" values, especially from the other functions mentioned at https://github.com/openjdk/jfx/pull/1442/#discussion_r1570948582. That was too defensive, as only `requestedSteps` AKA `length` can be out of range.
> 
> OK, I seem to have understood "Also, this doesn't follow the usual pattern of checking for integer overflow" by Kevin wrong. I googled the Java way of the usual pattern for Integer overflow. Since Java 8, there is `Math.addExact`. I thought, that this was meant. -- I found it from https://stackoverflow.com/a/3001879/873282. (Inlining the code of `Math.addExact` seems to have negative performance impact.)
> 
> The proposed code works OKish if strings are not in length area of Integer.MAX_VALUE. I think, we can safely assume that. - It however returns more characters if start is greater then 0. Example: I request start 2, length of 5, but maximum end index  of 3. Then 3 should be returned, not 7.
> 
> ---
> 
> I changed the code accordingly. Also added a comment when an overflow might happen. From the discussion here, it seems, we can ignore these cases.
> 
> Note that the old code returned `0` if `start` was negative. New might return some negative value if `start` is negative enough. However, did not seem to happen, because otherwise, IndexOutOfBounds exceptions might have been seen.

Thank you!  Somehow the code looks much cleaner and clearer now.

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/1442#discussion_r1574951836