On Wed, 18 Feb 2026 08:28:56 GMT, John Hendrikx <[email protected]> wrote:
>> OK, I am not against doing this in principle. But it's a new, non-trivial >> algorithm that needs to have additional tests and the implementation needs >> to be reviewed. The PR description should also change to reflect this added >> complexity. > >> OK, I am not against doing this in principle. But it's a new, non-trivial >> algorithm that needs to have additional tests and the implementation needs >> to be reviewed. The PR description should also change to reflect this added >> complexity. > > The tests are there to proof that it parses doubles correctly. The algorithm > can be treated as a black box. It doesn't need to be exhaustively checked, > that has already been done far more thoroughly than we can hope to do -- it's > like having to prove that some complex sorting algorithm works; you don't > need to fully understand the algorithm, just verify some edge cases, to proof > you didn't make a mistake "copying" the proven algorithm. > > I checked what Michael copied, and I didn't see any mistakes. The test cases > (and probably numerous dependent tests in the rest of FX) proof that it works. Let me explain. One of the reasons we do code review is to understand the change sufficiently enough in order to be able to maintain it, possibly over decades. the code review process requires non-zero effort, so please be patient with us (and thanks for reviewing it!) Many thanks to @mstr2 for including the reference to the original papers, but still - it is a novel, non-trivial implementation, which raises the bar significantly. I've asked a few questions, it would be nice to see them addressed at some point: - some measurements of the improvement in parsing performance - more tests to demonstrate the correct behavior, esp. near transition points - updated PR description (and possibly JBS as well) mentioning the new algorithm - document the changes in behavior (a whitespace handling is one, are there other changes?) - do the behavior differences cross the threshold for this to require a CSR? ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/2069#discussion_r2823888953
