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

Why does netcdf_mem.h #include netcdf.h? #1148

Closed
czender opened this issue Sep 23, 2018 · 15 comments
Closed

Why does netcdf_mem.h #include netcdf.h? #1148

czender opened this issue Sep 23, 2018 · 15 comments

Comments

@czender
Copy link
Contributor

czender commented Sep 23, 2018

What purpose does this code in netcdf_mem.h serve?

#ifndef NETCDF_MEM_H
#define NETCDF_MEM_H 1

#include <netcdf.h>
@DennisHeimbigner
Copy link
Collaborator

This is a common idiom in C programming. It prevents a header from
being included twice, which can happen sometimes. The first time the compiler
sees the header, netcdf_mem.h, NETCDF_MEM_H is undefined, so the contents
of netcdf_mem.h are processed by the compiler. I the compiler encounters
netcdf_mem.h again, NETCDF_MEM_H is now defined, so the contents
of netcdf_mem.h are skipped. Is that what you are referring to?

@czender
Copy link
Contributor Author

czender commented Sep 23, 2018

Dennis, I'm referring specifically to the line

#include <netcdf.h>

netcdf_mem.h is basically a red-headed stepson of netcdf.h.
When does it make sense to #include netcdf_mem.h without already having #included netcdf.h?

@DennisHeimbigner
Copy link
Collaborator

Probably historical. Not needed especially since netcdf_mem.h does not
appear to include any definitions from netcdf.h
I will zap it.

@czender
Copy link
Contributor Author

czender commented Nov 30, 2018

Answering my own question from the OP:

netcdf_mem.h depends on netcdf.h for this definition:

#define EXTERNL MSC_EXTRA extern /**< Needed for DLL build. */
Without that definition netcdf_mem.h is invalid C/CPP.

So please keep the #include <netcdf.h> or make sure EXTERNL is defined in netcdf_mem.h otherwise autoconf tests for netcdf_mem.h return confusing values (present but does not compile), as we just learned in NCO.

@DennisHeimbigner
Copy link
Collaborator

We need to state that all of the secondary netcdf_xxx.h files require
the inclusion of netcdf.h. Where xxx is _mem, _par, etc.

@mathomp4
Copy link

mathomp4 commented Dec 3, 2018

Odd query from a Fortran programmer still learning how C operates: Why doesn't netcdf.h have #include "ncexternl.h"?

Or, I suppose, why is EXTERNL (re-?)defined in some header files but others include ncexternl.h?

@DennisHeimbigner
Copy link
Collaborator

This was an historical attempt to factor out the EXTERNL stuff into a separate header.
Probably not needed, but the rule was intended to be that ncexternl.h
would be used for non-public header files in the include directory.

@czender
Copy link
Contributor Author

czender commented Jan 29, 2019

Umm, let me clarify that I think removing

#include <netcdf.h>
from netcdf_mem.h was a mistake because it breaks some 4.6.2 build configurations that (apparently) Unidata does not test, and it should be restored ASAP. In summary:

Including a definition for EXTERNL in netcdf_mem.h is up to Unidata, not NCO. The latest NCO snapshot builds for me with 4.6.2 on Linux, and I think it does not build for you or Matthew Thompson because netCDF 4.6.2 will not configure and build correctly with your compilers. Apparently some build systems do not mind that EXTERNL is not defined in netcdf_mem.h, because it is always defined when actually used. In other words, the problem, I think, is that autoconf tools (like configure and config.test) want each header file to be self-contained and netcdf_mem.h no longer is in 4.6.2. Unfortunately, I feel some guilt over the current situation because I asked what the purpose of having #include netcdf.h in netcdf_mem.h was, and they then removed it because none of us understood the implications of doing so. Apparently their test configurations miss this failure so please report it to Unidata. I will also forward them this message.

The full thread is here:
https://sourceforge.net/p/nco/discussion/9831/thread/e6faa0b8c1/?limit=25

@DennisHeimbigner
Copy link
Collaborator

Recall that the include/netcdf*.h files are installed files so that they represent
the external API for the netcdf library. I did a quick check and none of them
(except netcdf_f.h) have an include of netcdf.h.
The assumption for all of these files is that the client program includes
netcdf.h and then follows that with any other netcdf_XXX.h files necessary.
That is, they represent an extension of sorts of netcdf.h.
Under that assumption, including netcdf.h in any of the netcdf_XXX.h files
is redundant.

@WardF
Copy link
Member

WardF commented Jan 29, 2019

Good enough for me.

@WardF
Copy link
Member

WardF commented Jan 29, 2019

I’ll see about getting a fix in shortly.

@WardF
Copy link
Member

WardF commented Feb 16, 2019

@czender is restoring the EXTERNL definition to netcdf_mem.h sufficient to fix this issue? I'm preparing the 4.6.2.1 release to go up ASAP, and have restored this definition (currently extant in branch v4.6.2.1-dev.wif and will propagate back into master shortly; it will also be in the v4.6.2.1 tag when it is created).

@czender
Copy link
Contributor Author

czender commented Feb 16, 2019

I think so

@edwardhartnett
Copy link
Contributor

I think this issue should be closed.

@edwardhartnett
Copy link
Contributor

I still think this issue should be closed. ;-)

@czender czender closed this as completed Apr 28, 2022
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

No branches or pull requests

5 participants