Skip to content

Conversation

@winfriedgerlach
Copy link

  • both new BigInteger() and Long.parseLong() accept strings with + prefix, there is no need to remove anything (maybe the confusion came from an error in the JavaDoc, I noticed that in Java 11 the JavaDoc for BigInteger(String) doesn't mention the + sign while the JavaDoc for BigInteger(String, int) is correct; the JavaDoc was fixed in later Java versions)
  • the extra checks for . or non-digits also make no sense, because they will lead to a NumberFormatException anyway
  • there should be a slight performance benefit as well, but microbenchmarking showed that this is insignificant --> correct semantics are more important here

Copy link
Contributor

@laurentschoelens laurentschoelens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same remark as eclipse-ee4j/jaxb-ri#1874

I'd be happy to see every use case handled in the removed function in order to check if nothing is broken.

Also please update copyrights header regarding Oracle or Eclipse situation you're in

@winfriedgerlach winfriedgerlach force-pushed the remove-redundant-method-calls-in-DataTypeConverter branch from faaf23c to c131097 Compare November 12, 2025 16:18
@winfriedgerlach
Copy link
Author

winfriedgerlach commented Nov 12, 2025

@laurentschoelens phew, if I would have known about this rabbit hole beforehand, I wouldn't have touched anything...
Turns out that both the old code as well as the new code is incorrect. The old code performs correct validation, but only if the integer starts with + and only for the first digit after the +... The rest of the number is parsed using java.lang.Character#isDigit(int), which accepts 360 other kinds of digits from Unicode 🙈

See https://github.com/jakartaee/jaxb-api/pull/328/files#diff-66f7e84e30bf1ef955f70317f20c9aae7fd656516af68e7c0d816f24df97d80b

I see 3 ways to go from here:

  1. leave implementation as it is, people are used to it and nobody seems to have an urgent problem with it
  2. apply my suggested code change, which doesn't really make things worse (only for the edgy edge case that only the first digit after a leading + sign matters...) but code a bit simpler and a tiny bit faster
  3. implement proper character validation, i.e. check all digits for validity in the XML sense before handing over to BigInteger/Long; adds performance overhead

@laurentschoelens
Copy link
Contributor

I'm a bit precautionous about that kind of changes that would lead to potential breaking in api (see latest jaxb-api release regarding base64 decoding)

I would just have added all test cases that gets 100% coverage with actual code (and specially with removed function) and see that nothing gets worse with your PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants