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 accessing the SpatialRef of GCPs. #397

Merged
merged 2 commits into from
May 17, 2023

Conversation

metasim
Copy link
Contributor

@metasim metasim commented May 16, 2023

  • I agree to follow the project's code of conduct.
  • I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

Also removes trailing use of old fixture! macro originally removed in #342

@metasim
Copy link
Contributor Author

metasim commented May 16, 2023

cc: @mwielocha

use crate::vsi::unlink_mem_file;
use crate::DriverManager;
use std::path::Path;

#[cfg(feature = "ndarray")]
use ndarray::arr2;

macro_rules! fixture {
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit confused here, but 😍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lnicola It should have been removed in #342, but somehow was missed.

return None;
}

unsafe { SpatialRef::from_c_obj(c_ptr) }.ok()
Copy link
Member

@lnicola lnicola May 17, 2023

Choose a reason for hiding this comment

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

I wonder if this is leaking the OGRSpatialReferenceH (check out the implementation of from_c_obj). No, it doesn't.

Does foreign_types really solve issues like this? I only skimmed your changes, but it would be great if it did.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lnicola It solves it in the sense that we can more accurately reflect the owned vs borrowed semantics described in the GDAL docs, in a principled manner. After digging into the Layer and MDArray bindings, I'm pretty sure we're either leaking resources or dangerously borrowing them (no proof yet, just suspicion). Using foreign_types would also replace GeometryRef and OwnedLayer.

Copy link
Member

Choose a reason for hiding this comment

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

That would be awesome, I'll try to look into it.

@lnicola
Copy link
Member

lnicola commented May 17, 2023

bors r+

bors bot added a commit that referenced this pull request May 17, 2023
397: Support for accessing the `SpatialRef` of GCPs. r=lnicola a=metasim

- [X] I agree to follow the project's [code of conduct](https://github.com/georust/gdal/blob/master/CODE_OF_CONDUCT.md).
- [X] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users.
---

Also removes trailing use of old `fixture!` macro originally removed in #342

Co-authored-by: Simeon H.K. Fitch <[email protected]>
@bors
Copy link
Contributor

bors bot commented May 17, 2023

Build failed:

@lnicola
Copy link
Member

lnicola commented May 17, 2023

These clippy lints are more and more ridiculous, I swear.

@lnicola
Copy link
Member

lnicola commented May 17, 2023

CC bitflags/bitflags#354

@metasim
Copy link
Contributor Author

metasim commented May 17, 2023

These clippy lints are more and more ridiculous, I swear.

Damnit. I'm running rustc 1.69 (same as bors), running cargo clippy --all-targets -- -D warnings, and not getting any warnings. 😠

@lnicola
Copy link
Member

lnicola commented May 17, 2023

It doesn't fail on your branch, it fails after bors merges it into master, probably because we updated bitflags in the meantime. You might have started from a stale branch.

@metasim
Copy link
Contributor Author

metasim commented May 17, 2023

bors retry

bors bot added a commit that referenced this pull request May 17, 2023
397: Support for accessing the `SpatialRef` of GCPs. r=lnicola a=metasim

- [X] I agree to follow the project's [code of conduct](https://github.com/georust/gdal/blob/master/CODE_OF_CONDUCT.md).
- [X] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users.
---

Also removes trailing use of old `fixture!` macro originally removed in #342

Co-authored-by: Simeon H.K. Fitch <[email protected]>
@bors
Copy link
Contributor

bors bot commented May 17, 2023

Canceled.

@metasim
Copy link
Contributor Author

metasim commented May 17, 2023

bors retry

@bors
Copy link
Contributor

bors bot commented May 17, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit cf3deae into georust:master May 17, 2023
@metasim metasim mentioned this pull request May 18, 2023
@metasim metasim deleted the feature/gcp branch May 18, 2023 20:59
@lnicola
Copy link
Member

lnicola commented May 19, 2023

For some reason, I'm still getting these clippy warnings locally, even with #[allow(clippy::assign_op_pattern)].

@metasim
Copy link
Contributor Author

metasim commented May 19, 2023

For some reason, I'm still getting these clippy warnings locally, even with #[allow(clippy::assign_op_pattern)].

I had the other problem... even with everything updated I couldn't get the warnings to occur. :/

@metasim
Copy link
Contributor Author

metasim commented May 19, 2023

image image

🤷

@lnicola
Copy link
Member

lnicola commented May 19, 2023

It's supposed to work, and it's fine on CI. Must be something weird on my system.

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