On Tue, 29 Oct 2024 14:03:17 GMT, Raffaello Giulietti <rgiulie...@openjdk.org> wrote:
>> Joe Darcy has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Add support for proper String -> Float16 conversion. > > src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Float16.java line > 445: > >> 443: >> 444: if (trialResult == 0.0 // handles signed zeros >> 445: || Math.abs(trialResult) > (65504.0 + 32.0) || // >> Float.MAX_VALUE + ulp(MAX_VALUE) > > Suggestion: > > || Math.abs(trialResult) > (65504.0 + 16.0) || // Float.MAX_VALUE > + ulp(MAX_VALUE) / 2 When writing the code, I didn't think through all the possible boundary conditions near Float16.MAX_VALUE + 0.5*ulp(Float16.MAX_VALUE) so I bumped out the overflow limit. > src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Float16.java line > 504: > >> 502: // without the period. >> 503: hexSignificand.append(s.substring(digitStart, >> periodIndex)); >> 504: hexSignificand.append(s.substring(periodIndex + 1, >> pIndex)); > > Suggestion: > > hexSignificand.append(s, digitStart, periodIndex); > hexSignificand.append(s, periodIndex + 1, pIndex); Good refactoring; thanks for the suggestion. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/21574#discussion_r1821885437 PR Review Comment: https://git.openjdk.org/jdk/pull/21574#discussion_r1821887086
