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

Read Short as u16, SignedShort as i16 #235

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

weiji14
Copy link
Contributor

@weiji14 weiji14 commented Jul 22, 2024

Previously, Short values were read incorrectly as u32 instead of u16, and SignedShort values were read incorrectly as i32 instead of i16.

This PR does the following:

  • Add TiffFormatError::ShortExpected enum variant
  • Correctly cast Type::SHORT as u16 values, and support casting Short to other unsigned types via the into_u32_vec and into_u64_vec methods.
  • Correctly cast Type::SSHORT as i16 values

Fixes #204

@weiji14 weiji14 changed the title Read Short as u16 Read Short as u16, SignedShort as i16 Jul 22, 2024
@weiji14 weiji14 marked this pull request as ready for review July 22, 2024 05:13
@fintelia
Copy link
Contributor

The hard part is still this concern:

This makes sense to fix, though the annoying part is figuring out whether anywhere else in the code is relying on the current behavior. Or whether it is likely to break any downstream users and thus might warrant additional caution

@weiji14
Copy link
Contributor Author

weiji14 commented Jul 22, 2024

The hard part is still this concern:

This makes sense to fix, though the annoying part is figuring out whether anywhere else in the code is relying on the current behavior. Or whether it is likely to break any downstream users and thus might warrant additional caution

A tiff::decoder::ifd::Value would still need to be cast into a primitive type using into_u16, into_u32 or some other method no? My impression was that the change here would mean a user who is already doing Value.into_u16() would save some memory by not going through u16 -> u32 -> u16 as before, but just u16 all the way. That said, I do agree that this would warrant a minor version bump (or whatever cargo-semver-checks suggests 😉).

@fintelia
Copy link
Contributor

Cargo semver-checks doesn't capture behavior changes. If there's downstream code that assumes (perhaps via if let) that it'll get a Value::Unsigned() for something and we switch the decoder to produce Value::Short() instead, the code will stop working.

@weiji14
Copy link
Contributor Author

weiji14 commented Jul 24, 2024

Ok, what would you recommend then to mitigate against downstream breakages? Would a minor version bump to v0.10.0 (with a changelog entry that mentions this breaking change) not suffice?

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.

Tags of type Short are read as Unsigned
2 participants