Skip to content

Better handling of error conditions due to map_setup call#4347

Merged
PaulWessel merged 2 commits intomasterfrom
map_setup-return
Oct 19, 2020
Merged

Better handling of error conditions due to map_setup call#4347
PaulWessel merged 2 commits intomasterfrom
map_setup-return

Conversation

@PaulWessel
Copy link
Member

This PR is addressing the issues raised in #4344. We had a convoluted set of error checks and reporting that gave both correct error messages and confusing wrong references to other things due to a mix of internal and "official" error codes. This PR changes this to a simpler reporting. I was unable to reproduce the SEGV in the OP examples but I believe it was related to bad array indices into the gmt_error_string[] array. This hopefully goes away - if not we will follow up.

We had a convoluted set of error checks and reporting that gave both correct error messages and confusing wrong references to other things due to a mix of internal and "official" error codes.  This PR changes this to a simpler reporting.  I was unable to reproduce the SEGV in the OP examples but I believe it was related to bad array indices into the gmt_error_string[] array.  This hopefully goes away - if not we will follow up.
@PaulWessel PaulWessel self-assigned this Oct 19, 2020
Copy link
Contributor

@anbj anbj left a comment

Choose a reason for hiding this comment

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

Prints error message and quits, and segv is gone - perfect.

@PaulWessel PaulWessel merged commit 0a37e1a into master Oct 19, 2020
@PaulWessel PaulWessel deleted the map_setup-return branch October 19, 2020 16:51
@seisman
Copy link
Member

seisman commented Oct 23, 2020

$ gmt info file-do-not-exist
gmtinfo [ERROR]: Cannot find file file-do-not-exist

The above command returns error code 71 before this PR, but now it returns 72. Is it expected?

@PaulWessel
Copy link
Member Author

Impressed you noticed! I added one more error code and I added it alphabetically so the numbers did change. Since we do not even publish those values and what they are, does it matter?

@seisman
Copy link
Member

seisman commented Oct 23, 2020

A PyGMT test fails due to the change of the error code. It doesn't matter. It's a bad idea to check those non-zero status codes. Will fix it in PyGMT.

@joa-quim
Copy link
Member

joa-quim commented Oct 23, 2020

I notice the same in a Julia test. Had to fix the error number as I can't break the error message into a non-numeric string.

Edit: But shit, I have to remove this test as users that run the tests will have different results depending on their GMT version.

@PaulWessel
Copy link
Member Author

Would you prefer that I move that new error constant to the end of the list instead of having it sorted by name? I think though as long as we are not documenting what these constants are as part of the API then they are basically not defined for users to use.

@joa-quim
Copy link
Member

Better not care about this. I'll have to find a way of not reporting the error number, which doesn't inform much anyway.

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.

4 participants