Skip to content

Add float opcode handlers - #1127

Merged
popematt merged 21 commits into
amazon-ion:ion11from
austnwil:austnwil/ion11-float-handlers-2
Oct 17, 2025
Merged

popematt merged 21 commits into
amazon-ion:ion11from
austnwil:austnwil/ion11-float-handlers-2

Conversation

@austnwil

Copy link
Copy Markdown
Contributor

Description of changes:

  • Adds handlers for fixed-width float opcodes (0x6a-0x6d)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@austnwil
austnwil marked this pull request as ready for review October 16, 2025 20:13
@codecov

codecov Bot commented Oct 16, 2025 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 89.85507% with 7 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (ion11@10d510d). Learn more about missing BASE report.

Files with missing lines Patch % Lines
...ion/bytecode/bin11/bytearray/OpcodeHandlerTable.kt 0.00% 4 Missing ⚠️
...main/java/com/amazon/ion/bytecode/NumericReader.kt 86.36% 0 Missing and 3 partials ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             ion11    #1127   +/-   ##
========================================
  Coverage         ?   68.79%           
  Complexity       ?     6060           
========================================
  Files            ?      196           
  Lines            ?    24837           
  Branches         ?     4346           
========================================
  Hits             ?    17087           
  Misses           ?     6424           
  Partials         ?     1326           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@popematt popematt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

While the overall code looks correct, I'd like to suggest a slightly different factoring that has a cleaner separation of concerns and will make it easier to re-use some of the code.

I'd recommend creating a utility function such as Short.asHalfToFloat(), and then in the opcode handlers, you have something like this:

val floatValue = byteArray.readShort(position).asHalfToFloat()
BytecodeEmitter.emitFloatValue(destination, floatValue)
return 2

Why? This keeps the float conversion logic separated from the concerns of reading from the byte array, and it means that all of our utility functions are focused with a single responsibility. And when we implement bytecode generators for e.g. ByteBuffer or InputStream, we can read an integer value from those, and then re-use the same conversion logic to get the respective float or double values.

p += hourValueAndLength.toInt() and 0xFF
hour = (hourValueAndLength shr 8).toInt()
if (p >= end) {
// TODO: ion-java#1114

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's okay that you've included this change, but don't go out of your way to find things like this that are not directly related to the code you're working on. It's good to keep the PRs focused.

@austnwil

Copy link
Copy Markdown
Contributor Author

I'd recommend creating a utility function such as Short.asHalfToFloat(), and then in the opcode handlers, you have something like this:

Good suggestion, definitely more organized this way

Comment thread src/test/java/com/amazon/ion/bytecode/NumericReaderTest.kt Outdated
Comment thread src/test/java/com/amazon/ion/bytecode/bin11/bytearray/FloatOpcodeHandlerTest.kt Outdated
@popematt
popematt merged commit 7b6936e into amazon-ion:ion11 Oct 17, 2025
28 of 43 checks passed
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