Skip to content

Add support for short/float/double arrays #197

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.

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 5 commits into
base: 2.x
Choose a base branch
from

Conversation

lukehutch
Copy link

Proposed fix for #196

Created with Claude Code, so should be double-checked :-)

@@ -61,6 +61,9 @@ public abstract class ValueLocatorBase
public final static int SER_INT_ARRAY = 5;
public final static int SER_LONG_ARRAY = 6;
public final static int SER_BOOLEAN_ARRAY = 7;
public final static int SER_SHORT_ARRAY = 38;
Copy link
Member

Choose a reason for hiding this comment

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

Needs to be consecutive.

Copy link
Member

Choose a reason for hiding this comment

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

... which gets tricky wrt whether to renumber entries or move these to later.

I think renumbering is needed, so move SER_TREE_NODE etc up by number of added entries needed.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I saw that, and had the same thought, but I figured you'd know which of the two options (renumbering all later constants, or moving the other arrays to the end, which is ugly) would make the most sense.

@cowtowncoder
Copy link
Member

cowtowncoder commented Jun 17, 2025

@lukehutch Did you review it yourself? Or run tests? (although TBH Sonatype Snapshot repo has issues so perhaps it is difficult). Looks like unit tests fail.

Overall makes sense, GenAI coding agents are pretty good at following patterns. But there are some aspects that need changing; adding comments.

p.nextToken();
}

java.util.List<Long> values = new java.util.ArrayList<>();
Copy link
Member

Choose a reason for hiding this comment

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

Very inefficient, no. Have a look at how int[] is handled.

case JsonTokenId.ID_END_ARRAY:
break main_loop;
default:
throw new JSONObjectException("Failed to bind `long` element if `long[]` from value: "+
Copy link
Member

Choose a reason for hiding this comment

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

Funny. Copied typo from int[] case :)

("if" -> "of")

(will fix separately)

p.nextToken();
}

java.util.List<Boolean> values = new java.util.ArrayList<>();
Copy link
Member

Choose a reason for hiding this comment

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

Same as above, need to build an array to avoid all the boxing, unboxing.

_generator.writeFieldName(fieldName);
writeDoubleArrayValue(v);
}

Copy link
Member

Choose a reason for hiding this comment

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

This class looks good.

@lukehutch
Copy link
Author

I couldn't run the tests, and yes, this is going to be imperfect, but I figured I'd submit it as a starting point to save you a lot of work. Thanks!

@lukehutch
Copy link
Author

By the way you could also continue to iterate with Claude Code -- probably the issues you found could be fixed with a couple more prompts consisting of one or two sentences.

@cowtowncoder
Copy link
Member

cowtowncoder commented Jun 18, 2025

By the way you could also continue to iterate with Claude Code -- probably the issues you found could be fixed with a couple more prompts consisting of one or two sentences.

My experience with Wind Surfer was... not great so I am bit of GenAI skeptic.

While tools are impressive in their own right, the one and only piece of code in Jackson that I auto-generated, turned out to be the first bug reported for 2.19.0.... :-D
(yeah I should have thought through recursive handling to see the issue but it "looked ok" -- and that's what GenAI optimizes: things that appear similar to what might pass. But I digress).

@lukehutch
Copy link
Author

Windsurf is pretty bad. Claude Code is almost magical, especially for tasks like this that really just require pattern matching, as long as the codebase is not enormous (in which case you run into context window size issues, where it will forget state each time it has to compact the context).

@JooHyukKim
Copy link
Member

@lukehutch were you able to use Claude code to look at current failures? Since we are at public repository, I suppose it should be able to fetch CI failures as well?

@lukehutch
Copy link
Author

I didn't have it look at test results or anything like that. I had it identify missing support for arrays of any of the 7 primitive types. It correctly identified that arrays of char are treated as strings, and some other special case handling.

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.

3 participants