Skip to content

Fixes Implicit narrowing conversion in compound assignment FloatToDecimal #1450

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: 2.x
Choose a base branch
from
Open

Fixes Implicit narrowing conversion in compound assignment FloatToDecimal #1450

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/main/java/com/fasterxml/jackson/core/io/schubfach/FloatToDecimal.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,14 +405,14 @@ private static int rop(long g, long cp) {
/*
Formats the decimal f 10^e.
*/
private int toChars(int f, int e) {
private int toChars(long f, int e) {
Copy link
Member

Choose a reason for hiding this comment

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

I can't find any calls to this method where the input f is long - they are all int. I think there will be a performance penalty here as there will need to be an implicit cast from int to long on all calls to this method after this change.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @pjfanning

Thank you for the respond.

The reason I changed the parameter type from int to long was to address a potential integer overflow issue when handling large float values, particularly during conversion to a decimal representation. However, I understand the concern regarding the potential performance impact due to implicit casting from int to long on all existing calls to this method. If all current usages only involve int, I’m open to alternative approaches such as:

  • Introducing an overloaded method toChars(long f, int e) without removing the existing int version, to preserve performance for current usage patterns.
  • Re-evaluating whether long is truly necessary, or if overflow can be handled with validation before the call.

I’m happy to adjust the fix based on what would be best for both performance and maintainability. Please let me know your preferred direction.

Copy link
Member

Choose a reason for hiding this comment

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

I guess the start is to provide a broken test case. I'd prefer not to deal with theory and more interested in actual test cases.

Copy link
Member

@pjfanning pjfanning Jun 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

the code change doesn't even compile

Error:  COMPILATION ERROR : 
Error:  /home/runner/work/jackson-core/jackson-core/src/main/java/com/fasterxml/jackson/core/io/schubfach/FloatToDecimal.java:[415,63] incompatible types: possible lossy conversion from long to int

/*
For details not discussed here see section 10 of [1].

Determine len such that
10^(len-1) <= f < 10^len
*/
int len = flog10pow2(Integer.SIZE - numberOfLeadingZeros(f));
int len = flog10pow2(Long.SIZE - numberOfLeadingZeros(f));
if (f >= pow10(len)) {
len += 1;
}
Expand All @@ -437,7 +437,7 @@ private int toChars(int f, int e) {
floor(f / 10^8) = floor(1_441_151_881 f / 2^57)
*/
int h = (int) (f * 1_441_151_881L >>> 57);
int l = f - 100_000_000 * h;
int l = (int) (f - 100_000_000L * h);

if (0 < e && e <= 7) {
return toChars1(h, l, e);
Expand Down
Loading