On Fri, 19 Apr 2024 17:55:12 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> Oliver Kopp has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix test
>
> 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. 😅
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.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1442#discussion_r1573308526
