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 compiler warnings in grdppa.c #122

Merged
merged 2 commits into from
Oct 15, 2018
Merged

Fix compiler warnings in grdppa.c #122

merged 2 commits into from
Oct 15, 2018

Conversation

remkos
Copy link
Contributor

@remkos remkos commented Oct 14, 2018

Because I got a lot of warnings in compiling grdppa.c
Biggest problem was the missing #include gmt_internals.h

@PaulWessel
Copy link
Member

So far, only gmt_*.c library files include gmt_internals.h and none of the modules do. What does grdppa need that is not accessible via gmt_prototypes.h (which is included in gmt_dev.h for module use)? TO keep this separation up we sometimes promote functions that are in gmt_internals.h to gmt_prototypes.h so that we don't need to add one-time EXTERM_MSC lines in the modules. As I write this, gmt_prototypes.h is not a great self-intuitive name for what it means...

@joa-quim
Copy link
Member

Hmm, but I don't need to #include "gmt_internals.h" and neither needs CI because the PRs passed the build step.

@remkos
Copy link
Contributor Author

remkos commented Oct 15, 2018

It does compile without #include gmt_internals.h, but then there is still a (reasonable) warning:

[ 70%] Building C object src/CMakeFiles/supplib.dir/misc/grdppa.c.o
src/misc/grdppa.c:503:2: warning: implicit declaration of function 'gmtlib_grd_flip_vertical' is invalid
      in C99 [-Wimplicit-function-declaration]
        gmtlib_grd_flip_vertical (G->data, G->header->mx, G->header->my, 0, sizeof(G->data[0]));
        ^
1 warning generated.

Obviously, gmtlib_grd_flip_vertical needs a prototype. I'll add it to gmt_prototypes.h then.

@remkos
Copy link
Contributor Author

remkos commented Oct 15, 2018

In fact, no ... if you use a gmtlib routine, there is no way avoiding #include gmt_internals.h or EXTERN_MSC.

@remkos
Copy link
Contributor Author

remkos commented Oct 15, 2018

So EXTERN_MSC added. But I don't like it. I prefer we use gmt_internals.h whenever we need any gmtlib routine.

@PaulWessel
Copy link
Member

I guess this depends on whether there is any benefit to us to keep functions only used within the GMT library as a separate include file (gmt_internals.h). If we were to include gmt_internals.h in gmt_dev.h, why even have separate gmt_prototypes.h and gmt_internals.h files? Isn't it better to rename any gmtlib_* function to gmt_* and promote to gmt_prototypes.h instead?

@PaulWessel PaulWessel merged commit 9725a67 into master Oct 15, 2018
@joa-quim
Copy link
Member

Yes, promoting it to gmt_* would be better. It's not rare that I need to do this operation to grids (mem layouts and stuff like that).

@remkos remkos deleted the remkos-grdppa-patch branch October 15, 2018 20:00
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.

None yet

3 participants