Skip to content
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

Parameter indexes cannot be stored safely as bytes #10

Open
sciwhiz12 opened this issue Dec 1, 2023 · 0 comments
Open

Parameter indexes cannot be stored safely as bytes #10

sciwhiz12 opened this issue Dec 1, 2023 · 0 comments
Labels
bug Something isn't working

Comments

@sciwhiz12
Copy link
Member

A glaringly large oversight I made when designing Feather was to assume that the parameter index (which is the , which can range from 0 to 255, fits in a byte. What I missed was that is true for unsigned bytes; Java's byte is a signed type, which ranges from -128 to 127. This causes issues if a parameter index actually falls after 127, which would cause a wraparound of the value (when cast to a byte) to a negative value.

While the various APIs don't seem to validate parameter indexes are positive (which is an oversight), the IO adapters do verify that; for example, the GSON adapter does that validate on read. This causes an asymmetrical problem where creating in-memory objects and exporting them is fine, but reading those exports will cause an error if they contain parameter indexes greater than 127.

The solution is to modify the API such that parameter indexes are stored in a larger data type than a byte. Using a short is acceptable, and would align with the intent of using byte to limit values to within the acceptable range of 0 to 255 for parameter indexes. However, I think usability can be improved by using an int instead, to avoid consumers needing to explicitly downcast parameter stored as ints to bytes. (We will incorporate parameter validation in the APIs, to ensure parameters are still within the 0-255 limit of the JVM.)

This will be a breaking API change, and as such slated for 2.0.0.

@sciwhiz12 sciwhiz12 added the bug Something isn't working label Dec 1, 2023
@AterAnimAvis AterAnimAvis mentioned this issue Apr 10, 2024
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant