Update proj::Proj constructors to return Result instead of Option.
#98
Conversation
frewsxcv
changed the title
proj::Proj constructors to return Result instead of Option.
|
Test failure look unrelated? |
frewsxcv
force-pushed
the
frewsxcv-create-error
branch
from
1a913d4
to
61fcbe0
Compare
src/proj.rs
Outdated
| let c_definition = CString::new(definition).ok()?; | ||
| fn transform_string(ctx: *mut PJ_CONTEXT, definition: &str) -> Result<Proj, ProjCreateError> { | ||
| let c_definition = CString::new(definition).map_err(|_| ProjCreateError::ArgumentNulError)?; | ||
| unsafe { proj_errno_reset(std::ptr::null()) }; |
There was a problem hiding this comment.
I'm not familiar with the proj API, but should this be pj_ctx_set_errno(ctx, 0) to match the thread-safe proj_context_errno below?
There was a problem hiding this comment.
@lnicola proj-sys doesn't expose proj_ctx_set_errno. Do you know why that might be?
There was a problem hiding this comment.
It looks like it's now called proj_context_errno_set, but that one's missing also.
There was a problem hiding this comment.
Do we even need to reset errno between calls?
There was a problem hiding this comment.
I understand the impulse to want to reset the errno - it's good hygiene in general. That said, in the current code, the two places where transform_string is used, we've just created the ctx, so I don't think it could have any stale error hanging around.
Maybe we could make that more obvious by creating the ctx here in this method rather than passing it in.
There was a problem hiding this comment.
Would it be hard to write a test that creates a context, executes something that fails, then something that succeeds to check for this? If it passes, we can probably skip clearing errno.
There was a problem hiding this comment.
Should this block me from merging?
To recap my understanding of things:
We're dealing with two entities:
Proj - represents a transform
Context - represents a threading context
Every Proj has a context. Even if you specify a "null" context then you are in fact still referring to a context - the "default" context. The same applies here, where a null Proj results in affecting the default Context (via https://github.com/OSGeo/PROJ/blob/master/src/ctx.cpp#L46)
So in passing a null ptr like this, we are potentially clobbering some state that we don't intend to.
e.g. Say I am using the default context somewhere, and concurrently I use this method to create a new Proj, then I have just clobbered whatever error state existed in the default context, which is potentially bad news.
So whatever solution you arrive at, I'm pretty confident that unsafe { proj_errno_reset(std::ptr::null()) }; needs to go away.
There was a problem hiding this comment.
In fact, I think that the only change that needs to happen is to delete that line. I don't think it needs to be replaced with anything at all. Since we've just created the ctx it can't have any error's set (At least, I think that's true...).
There was a problem hiding this comment.
Something I just learned. Every PJ object contains a reference to PJ_CONTEXT. And when we call proj_errno_reset(pj), it also resets the errno of the underlying context.
Notice that proj_errno_reset calls proj_context_errno_set.
There was a problem hiding this comment.
@michaelkirk @lnicola How do the latest commits look? Is this good to go?
Indeed! I'm looking into that. |
I feel pretty confident that the current output of CI is failing not due to your changes. Unfortunately, I don't have any leads on fixing the problem yet. 😓 You can look at #99 to see where I'm gathering data about the issue. |
Surface the underlying error from PROJ.
frewsxcv
force-pushed
the
frewsxcv-create-error
branch
from
02ea68e
to
1467e28
Compare
|
Squashed bors r=michaelkirk |
|
Build failed: |
Surface the underlying error from PROJ.