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 ISO c++ does not allow assigning string literals to char* #79

Conversation

peter-urban
Copy link
Collaborator

First partial fix for #77

This fixes the c++ warnings related to assigning constant strings to char*
The fix is simple and works in c and c++ :

  • replaced several char* with const char*

Most of the changes are unproblematic, since they only happen in gsw_check_functions.c

However, there is one change in gsw_saar_data.h (line 6,7 and 9). I wouldn't mind someone with better C command to double check this:

  • const char** is a mutable pointer to a const char* and not a const pointer to mutable char* right?
  • Also: While this is a minor change, it modifies the interface of gsw_get_version (const char** instead of char**). So it potentially require the libraries building on top of GSW-C to be updated? (e.g. GSW-Python)

@peter-urban peter-urban force-pushed the fix_iso_forbits_assigning_string_constant_to_'char_' branch from 74f3d7e to 8f4f33d Compare November 11, 2024 11:20
@efiring
Copy link
Member

efiring commented Nov 11, 2024

@peter-urban thank you for jumping on this. But why are about 500,000 changes showing up in gsw_saar_data.h?

@efiring
Copy link
Member

efiring commented Nov 11, 2024

gsw_get_version is not used in GSW-Python, and I doubt it is used anywhere else.
Nevertheless, we might as well get it right. @mauzey1, I suspect you are the expert here,
so if you have time, please advise on this PR and #77.

@peter-urban peter-urban force-pushed the fix_iso_forbits_assigning_string_constant_to_'char_' branch from 8f4f33d to 3271692 Compare November 11, 2024 19:38
@peter-urban
Copy link
Collaborator Author

@peter-urban thank you for jumping on this. But why are about 500,000 changes showing up in gsw_saar_data.h?

Oops, that was accidental auto-formatting. Should be fixed now.

@efiring
Copy link
Member

efiring commented Nov 11, 2024

Googling turned up a nice utility program:

$ cdecl explain "const char **x"
declare x as pointer to pointer to const char

@peter-urban

This comment was marked as outdated.

@peter-urban peter-urban force-pushed the fix_iso_forbits_assigning_string_constant_to_'char_' branch from 1bda1f0 to f2cea8e Compare November 11, 2024 22:25
@peter-urban
Copy link
Collaborator Author

peter-urban commented Nov 11, 2024

Reverted the changes I mentioned in the last comment.
It caused problems that I don't intend to fix in this PR.

(multiple_declaration failures when including gsw_saar_data.h because gsw_get_version is neither declared static or inline.)

@peter-urban
Copy link
Collaborator Author

However, I believe the function does exactly the same as it did before this PR, so it should be fine

@peter-urban peter-urban force-pushed the fix_iso_forbits_assigning_string_constant_to_'char_' branch from 1123de0 to b572173 Compare November 11, 2024 22:45
@peter-urban peter-urban force-pushed the fix_iso_forbits_assigning_string_constant_to_'char_' branch from b572173 to 6bab8d8 Compare November 12, 2024 20:02
@efiring
Copy link
Member

efiring commented Nov 12, 2024

@peter-urban what does your latest force-push do? And do we need to keep all 3 commits, given that numbers 2 and 3 just cancel out? I could use the "squash and merge" option to simplify the history.

to non-cost char pointers.
Fix: use const char* instead
@peter-urban peter-urban force-pushed the fix_iso_forbits_assigning_string_constant_to_'char_' branch from 6bab8d8 to 75c2c6d Compare November 12, 2024 20:34
@peter-urban
Copy link
Collaborator Author

Sorry for the force push. I just rebased the branch to the latest main not realizing that this may cause confusion after approval.

@peter-urban
Copy link
Collaborator Author

I squashed the commits; (I actually didn't know that this was possible until you just mentioned it)

@efiring efiring merged commit 130b114 into TEOS-10:main Nov 12, 2024
19 checks passed
@peter-urban peter-urban deleted the fix_iso_forbits_assigning_string_constant_to_'char_' branch November 12, 2024 21:22
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