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

Support for reading GeoKeyDirectory #19

Merged
merged 1 commit into from
Sep 23, 2024

Conversation

gschulze
Copy link
Collaborator

This PR implements support for reading the GeoKeyDirectory.

The structure is based on https://github.com/developmentseed/aiocogeo-rs/blob/main/src/geo_key_directory.rs, thanks kylebarron for the hint!

@weiji14 I split this off the coordinate transformation branch, as this is a prerequisite for debugging stuff conveniently in QGIS.

Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

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

Thanks @gschulze! Just some docstring and minor code suggestions/questions for now.

src/geo_key_directory.rs Show resolved Hide resolved
src/geo_key_directory.rs Show resolved Hide resolved
Comment on lines 511 to 470
count: u16,
offset: u16,
) -> TiffResult<u16> {
if location_tag.is_some() {
return Err(TiffError::FormatError(TiffFormatError::Format(format!(
"Key `{key_tag:?}` did not have the expected value type."
))));
}

if count != 1 {
Copy link
Member

Choose a reason for hiding this comment

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

Could use a ref here instead, and change all the *count to count above?

Suggested change
count: u16,
offset: u16,
) -> TiffResult<u16> {
if location_tag.is_some() {
return Err(TiffError::FormatError(TiffFormatError::Format(format!(
"Key `{key_tag:?}` did not have the expected value type."
))));
}
if count != 1 {
count: &u16,
offset: u16,
) -> TiffResult<u16> {
if location_tag.is_some() {
return Err(TiffError::FormatError(TiffFormatError::Format(format!(
"Key `{key_tag:?}` did not have the expected value type."
))));
}
if count != &1 {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can do that, but I'm unsure whether I completely understand the rationale. I think you want me to change it to a reference because of the contract of the equality operator: fn eq(&self, &other: Self): bool. Is there a performance impact to this, or is it just a matter of style? I can imagine the compiler will optimize this anyway. For me, *count == 1 is more intuitive than count == &1, as in my mind, I'm comparing values rather than references. Changing from count: u16 to count: &u16 also consumes more memory on the stack, four times as much on 64-bit systems. Anyway, I'm happy to change it if you want me to; I just wanted to share my thoughts. Is there any best-practices document you follow that I can relate to?

Copy link
Member

Choose a reason for hiding this comment

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

I'd love to hear rationale either way or see some resources on this, but for types that implement Copy I think it's simpler to not use a reference in the signature parameter

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok, I was approaching this more as a matter of style rather than performance. Still learning Rust on the go, and didn't realize that references to u16 would be stored as 64-bit (!!), so happy for you to stick with *count == 1.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm no expert on this topic either, but inspecting the code via Compiler Explorer might reveal more insights.

src/geo_key_directory.rs Outdated Show resolved Hide resolved
src/geo_key_directory.rs Outdated Show resolved Hide resolved
src/geo_key_directory.rs Outdated Show resolved Hide resolved
@gschulze
Copy link
Collaborator Author

@weiji14 Thanks for your comments; I updated the PR and incorporated most of your suggestions. See my comment on the usage of *count == 1 vs count == &1 above, though.

Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

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

Thanks again @gschulze, just one small doc suggestion, otherwise feel free to merge!

src/geo_key_directory.rs Show resolved Hide resolved
@weiji14 weiji14 added this to the v0.1.0 milestone Sep 22, 2024
@gschulze gschulze merged commit 77ec3f7 into georust:main Sep 23, 2024
2 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.

3 participants