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

ADApp/pluginSrc/NDFileTIFF.cpp: restore dedicated handling of 32-bit … #518

Closed
wants to merge 1 commit into from

Conversation

mp49
Copy link
Contributor

@mp49 mp49 commented Nov 11, 2024

…integers. Having the 32-bit case fall through to the 64-bit case resulted in corrupted data most of the time, and this restores previous behavior without affecting the new 64-bit support.

…integers. Having the 32-bit case fall through to the 64-bit case resulted in corrupted data most of the time, and this restores previous behavior without affecting the new 64-bit support.
@mp49
Copy link
Contributor Author

mp49 commented Nov 11, 2024

fixes #519

Copy link
Member

@MarkRivers MarkRivers left a comment

Choose a reason for hiding this comment

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

I think the original logic and this logic are both incorrect.

pAttribute->getValue() should be passed a datatype that matches the pointer it is passed. pAttribute->getValue() does the type conversion from the actual type of the attribute to datatype passed to it.

So the calls should be

pAttribute->getValue(NDAttrInt32, &value.i32);

and

pAttribute->getValue(NDAttrInt64, &value.i64);

The existing code will probably work OK on little-endian machines, but not on big-endian machines.

My modified code is still not perfect because UInt32 and UInt64 will be printed as if they are signed.

@MarkRivers
Copy link
Member

I have create a new PR that fixes the issues with this one, and adds testing. #520

@mp49
Copy link
Contributor Author

mp49 commented Nov 18, 2024

Closing this because #520 is a better fix

@mp49 mp49 closed this Nov 18, 2024
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