On Sat, 20 Apr 2024 14:50:15 GMT, Oliver Kopp <[email protected]> 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
