zfp/types.h header is not C++ safe

[peter-zajac]

When trying to use the zfp library for a C++ project, we have encountered a warning regarding the use of the __STDC_VERSION__ macro at the very beginning of the zfp/types.h header file. The problem is that the g++ compiler does not define this macro, because it considers itself to be a C++ and not a C compiler, as pointed out in the documentation in https://gcc.gnu.org/onlinedocs/cpp/Standard-Predefined-Macros.html:

This macro is not defined if the -traditional-cpp option is used, nor when compiling C++ or Objective-C.

According to the C++ standards, it is perfectly legit for a C++ compiler to leave __STDC_VERSION__ undefined.

Therefore, you should at least check whether __STDC_VERSION__ is defined first. Also, you might consider preceding this by an #if block that checks for __cplusplus >= 201103L, which would indicate a C++11 conforming implementation that offers the cstdint header along with the necessary typedefs, which you could use in analogy to the C99-conforming case.

And furthermore, in lines 62 and 67 of zfp/types.h, you are using #elif ZFP_LP64 instead of #elif defined(ZFP_LP64), which also leads to at least one warning regarding undefined macros.

[lindstro]

Let me make sure I understand the issue better. The C and C++ standards guarantee that any undefined macros are replaced with 0:

After all replacements due to macro expansion and the defined unary operator have been performed, all remaining identifiers are replaced with the pp-number 0, and then each preprocessing token is converted into a token.

(See C90/6.8.1, C99/6.10.1, C++98/16.1.) Thus, if __STDC_VERSION__ is undefined, then __STDC_VERSION__ >= 199901L is expanded to 0 >= 19901L, which evaluates to false, as intended. Is your concern that some compilers may issue a warning in spite of perfectly legal and idiomatic code?

You make a good point about C++11. zfp itself is written in C, but it's of course possible that types.h gets included in a C++ file. I will add appropriate code to handle the C++ case.

[peter-zajac]

First off, I want to apologize if my first post came over somewhat arrogant or if it sounded like I'm questioning your competence, because that was not my intention (and English is not my native language).

You are of course correct about that #if statement being perfectly legal C/C++ preprocessor code. However, as far as I have seen, it seems that you and the other authors consistently check whether macros are defined rather than relying on the "replaced-by-0" rule in all other cases. Therefore, I assumed that you simply have not known that __STDC_VERSION__ needs not be defined even by a standard-conforming C++ compiler -- at least I didn't know until yesterday when our nightly regression test system spit out that warning despite having 20+ years of C++ experience. Also it seems that g++ is the only (?) widely used C++ compiler that leaves __STDC_VERSION__ undefined, so I suppose that "(...) not g++ safe" might have been a more precise wording for the topic title.

In any case, we'll just put a pragma around the include of the zfp.h header to disable the warning.

[lindstro]

No need to apologize--I just wanted to better understand the issue and the circumstances that triggers it. I also suspect that g++ emits this warning only in very special cases, such as when -Wundef is turned on (which obviously is an explicit ask for such warnings). Using g++ -std=c++98 -pedantic -Wall -Wextra (with gcc 4.8.5), no warning is emitted for the following code:

int main()
{
#if __STDC_VERSION__ > 0
  return 1;
#else
  return 0;
#endif
}

There's little harm in adding defined(__STDC_VERSION__), and we generally try to ensure warnings are silenced, so I'll fix this. There's a balance to strike when rewriting code to silence picky compilers and ensuring code readability, which is why I'm usually a bit reluctant to address a warning without knowing what triggers it. Out of curiosity, what version of gcc are you using and what options have you enabled?

[peter-zajac]

I just had a discussion with my colleague who has pointed me to this warning in the first place and it seems that we had a misunderstanding regarding what his problem actually was.

First off, you are right about that one has to explictly add -Wundef to the command line parameters for g++ to trigger this warning. My colleague told me today that he had manually enabled this warning in our projects build system some time ago, whereas I erroneously assumed that it was one of the warnings that g++ has included in its -Wall settings in one of the previous major version updates. So again I feel the need to apologize for the confusion this has caused (this time I'm blaming my colleague though).

The problem that my colleague originally wanted to point out was not the warning itself, but rather the fact that a C compiler, which is used to compile the zfp library, uses the #if-block of the types.h header, whereas the C++ compiler, which is used to compile an application using the library, uses the #else block of the header, which might potentially lead to different type definitions.
However, in today's discussion we have come to the conclusion that this is probably a purely hypothetical problem, because even if the two code paths chosen by the C and C++ (gcc and g++) compilers lead to formally different types, e.g. long vs long long for int64, this shouldn't result in any linker errors or incorrect runtime behaviour, because the C linkage only cares for the byte-sizes of the corresponding types.

So again, sorry for any confusion that our internal misunterstanding might have caused.

[lindstro]

I can see how taking different paths between C and C++ can lead to different types and potential issues. This is something that is explicitly tested for in testzfp.cpp. In the rare event that types do differ, it's up to the user to supply compatible types via ZFP_INT64 etc. This is simply a consequence of C90 and C++98 not providing a mechanism like stdint for portable integer types. In the case of C99 and C++11, this should not be an issue (after I add appropriate C++ code to include cstdint).

[lindstro]

@peter-zajac I've pushed a fix on the develop branch that addresses the two issues you mentioned. Please let me know if this addresses your concerns.

[peter-zajac]

Sorry for the delayed reply, but we have been quite busy the past few days. I have talked to my colleague who has tested your fix and the g++ compiler has successfully stopped issuing warnings now. Thank you very much for your effort.

PS: Do you have an anticipated release date for the version 0.5.6 source tarball?

[lindstro]

I'm hoping we'll roll out 0.5.6 in the next couple of months. I unfortunately do not have a more precise date than that.