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

Fix array conversion #56

Merged
merged 4 commits into from
Jan 6, 2021
Merged

Fix array conversion #56

merged 4 commits into from
Jan 6, 2021

Conversation

urschrei
Copy link
Member

@urschrei urschrei commented Jan 6, 2021

My suspicion is that creating the raw pointer in the function call was causing it to be dropped prematurely, resulting in garbage data. See here for an analogous issue when getting a CString pointer. We avoid this by explicitly creating the pointer before the call. In addition, the doctest has been updated with a less complex example that doesn't use a proj string.

@urschrei
Copy link
Member Author

urschrei commented Jan 6, 2021

Bors try

bors bot added a commit that referenced this pull request Jan 6, 2021
@bors
Copy link
Contributor

bors bot commented Jan 6, 2021

try

Build succeeded:

@michaelkirk
Copy link
Member

bors try

Interesting! Let's gather some stats here.

bors bot added a commit that referenced this pull request Jan 6, 2021
@urschrei
Copy link
Member Author

urschrei commented Jan 6, 2021

If your run passes, I'm gonna contrive a doctest with different CRS and coordinates. I have my doubts about the validity of the old Stereo70 conversion values, but there's less value in a doctest that just reproduces an existing known-good test.

@bors
Copy link
Contributor

bors bot commented Jan 6, 2021

try

Build succeeded:

@urschrei
Copy link
Member Author

urschrei commented Jan 6, 2021

bors try

bors bot added a commit that referenced this pull request Jan 6, 2021
@bors
Copy link
Contributor

bors bot commented Jan 6, 2021

try

Build succeeded:

@urschrei
Copy link
Member Author

urschrei commented Jan 6, 2021

These are CRS and coordinate values from https://geodesy.noaa.gov/NCAT/.

WDYT?

Copy link
Member

@michaelkirk michaelkirk left a comment

Choose a reason for hiding this comment

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

Works every time, three of the times!

Thank you for (I think) tracking this down.

src/proj.rs Outdated Show resolved Hide resolved
src/proj.rs Show resolved Hide resolved
/// assert_approx_eq!(v[0].y(), 500027.77901023754f64);
/// let from = "EPSG:2230";
/// let to = "EPSG:26946";
/// let ft_to_m = Proj::new_known_crs(&from, &to, None).unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

Nice to have a more focused example in the docs. I tremble when faced with raw "proj script".

- better variable name
- remove test loop that appeared without explanation
@urschrei
Copy link
Member Author

urschrei commented Jan 6, 2021

bors r=michaelkirk

@bors
Copy link
Contributor

bors bot commented Jan 6, 2021

Build succeeded:

@bors bors bot merged commit a6cc3a1 into georust:master Jan 6, 2021
@urschrei urschrei deleted the fix_array branch January 6, 2021 19:12
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