Undefined configuration and compiler definitions cause warnings

[rikvdh]

Hi Nicholas,

I was compiling Mpack for a non-Linux platform today (non-GNU) and ran into a big list of warnings (46 to be exact). Since we want to compile our code with -Werror we really want to fix it. But we need your opinion on this.

See some of our warnings below this. We are using the amalgamation package, so line-numbers are crazy, but you will get the point. The compiler moans about the check within #if while the variable is not defined.

In the example of the first warning:

#if MPACK_STDLIB

should become:

#if defined(MPACK_STDLIB) && MPACK_STDLIB

but this creates a mess in the code. From my point of view, you want to use:

#ifdef MPACK_STDLIB

or

#if defined(MPACK_STDLIB)

but this changes the behavior when configuring Mpack, only setting config to 0 wouldn't suffice. You need to disable the config option completely.

We are using GCC 4.9.3 Linaro arm-none-eabi cross-tools.

I can fix this and make a PR for you. But (since it is your lib) I need your input on this.

Thanks,

Rik

/3rdparty/mpack/src/mpack/mpack.h:78:5: warning: "MPACK_STDLIB" is not defined [-Wundef]
 #if MPACK_STDLIB
     ^
/3rdparty/mpack/src/mpack/mpack.h:82:5: warning: "MPACK_STDIO" is not defined [-Wundef]
 #if MPACK_STDIO
     ^
/3rdparty/mpack/src/mpack/mpack.h:85:5: warning: "MPACK_SETJMP" is not defined [-Wundef]
 #if MPACK_SETJMP
     ^
/3rdparty/mpack/src/mpack/mpack.h:113:7: warning: "_MSC_VER" is not defined [-Wundef]
 #elif _MSC_VER
       ^
/3rdparty/mpack/src/mpack/mpack.h:192:5: warning: "MPACK_STDLIB" is not defined [-Wundef]
 #if MPACK_STDLIB
     ^
/3rdparty/mpack/src/mpack/mpack.h:224:5: warning: "MPACK_READ_TRACKING" is not defined [-Wundef]
 #if MPACK_READ_TRACKING && (!defined(MPACK_READER) || !MPACK_READER)
     ^
/3rdparty/mpack/src/mpack/mpack.h:227:5: warning: "MPACK_WRITE_TRACKING" is not defined [-Wundef]
 #if MPACK_WRITE_TRACKING && (!defined(MPACK_WRITER) || !MPACK_WRITER)
     ^
/3rdparty/mpack/src/mpack/mpack.h:231:9: warning: "MPACK_STDIO" is not defined [-Wundef]
     #if MPACK_STDIO
         ^
/3rdparty/mpack/src/mpack/mpack.h:234:9: warning: "MPACK_READ_TRACKING" is not defined [-Wundef]
     #if MPACK_READ_TRACKING
         ^
/3rdparty/mpack/src/mpack/mpack.h:237:9: warning: "MPACK_WRITE_TRACKING" is not defined [-Wundef]
     #if MPACK_WRITE_TRACKING
         ^
/3rdparty/mpack/src/mpack/mpack.h:559:5: warning: "MPACK_READ_TRACKING" is not defined [-Wundef]
 #if MPACK_READ_TRACKING || MPACK_WRITE_TRACKING
     ^
/3rdparty/mpack/src/mpack/mpack.h:559:28: warning: "MPACK_WRITE_TRACKING" is not defined [-Wundef]
 #if MPACK_READ_TRACKING || MPACK_WRITE_TRACKING
                            ^
/3rdparty/mpack/src/mpack/mpack.h:780:5: warning: "MPACK_WRITE_TRACKING" is not defined [-Wundef]
 #if MPACK_WRITE_TRACKING
     ^
/3rdparty/mpack/src/mpack/mpack.h:827:9: warning: "MPACK_SETJMP" is not defined [-Wundef]
     #if MPACK_SETJMP
         ^
/3rdparty/mpack/src/mpack/mpack.h:833:9: warning: "MPACK_WRITE_TRACKING" is not defined [-Wundef]
     #if MPACK_WRITE_TRACKING
         ^
/3rdparty/mpack/src/mpack/mpack.h:882:5: warning: "MPACK_STDIO" is not defined [-Wundef]
 #if MPACK_STDIO
     ^
/3rdparty/mpack/src/mpack/mpack.h:906:5: warning: "MPACK_SETJMP" is not defined [-Wundef]
 #if MPACK_SETJMP
     ^
/3rdparty/mpack/src/mpack/mpack.h:1203:5: warning: "MPACK_WRITE_TRACKING" is not defined [-Wundef]
 #if MPACK_WRITE_TRACKING
     ^
/3rdparty/mpack/src/mpack/mpack.h:1298:5: warning: "MPACK_READ_TRACKING" is not defined [-Wundef]
 #if MPACK_READ_TRACKING
     ^
/3rdparty/mpack/src/mpack/mpack.h:1351:9: warning: "MPACK_SETJMP" is not defined [-Wundef]
     #if MPACK_SETJMP

[ludocode]

Ah, yeah. Do you still need this fixed? I had originally wanted to avoid needing to support this warning. I don't necessarily agree with the warning anyway; the behaviour of #if on an undefined preprocessor macro has been well-defined for a long time, and in the case of using a macro as a boolean flag, there isn't really a risk of unexpected behaviour from it not being defined. It's kind of a silly warning. I also preferred using a plain #if because it allows macros to be defined as zero to disable a feature, and it catches attempts to change them without needing the verbose #if defined(X) && X. For example if it's defined 0 in one place, but then somewhere else defines it as 1 or vice-versa, you'll get a compiler error about redefining to something else. This isn't the case when using undefined versus defined blank as a boolean flag.

But, I've also seen -Wundef being commonly used especially on embedded platforms, and I've had to support it on other projects before. It sounds something like what you're working on, and is probably why it was defined on your project. So I suppose I should support it.

I agree that we can't just change to #ifdef since it would change behaviour if people were defining configuration options to 0. But I'm somewhat torn on which direction to go. We could either add a check to mpack-common.h that requires that configuration options be defined to 1 or not defined at all; then we can just use #ifdef. Or we can use #if defined(X) && X everywhere. Or we can add some code to mpack-common.h which undefs any of the macros defined to 0, then use #ifdef (probably the worst solution.) There are different pros and cons, and I haven't figured out which one to go with yet (or whether I should just leave it the way it is.)

In any case the boolean checks for _MSC_VER can be simply be changed to defined(_MSC_VER). That one is a straightforward fix.

[rikvdh]

I've added a PR for the _MSC_VER and also WIN32.

Is there a lot of legacy to support already? Perhaps is now the good time to make incompatible changes. I'd prefer #ifdef or #if defined()This keeps the code clean, prevents warnings on -Wundef. We could make a mpack-compat.h and leave it there which does:

#if defined(VAR) && !VAR
#error You must remove VAR in order to unset it
#endif

And leave it there for a few versions. This would at least have my preference.

[ludocode]

I was actually leaning the other direction, #undefing any that are defined to something that doesn't evaluate to true. This doesn't break backwards compatibility, and continues to allow them to be defined to 0. The mpack-config.h.sample should probably be written like this anyway:

#ifndef MPACK_WRITER
#define MPACK_WRITER 1
#endif

That way you can use the default config file, but override some by pre-defining them to zero in your project configuration or whatever.

[ludocode]

Fixed on branch develop in a3fd12f. The unit tests now build with -Wundef. Give it a try. (You can generate a replacement amalgamated mpack.{h,c} by running tools/package.sh. I probably won't be doing a new release for a few weeks.)

[rikvdh]

Looks fine to me. I've not tested it yet, but it seems correct. Perhaps it is a good idea to add #warning next to the #undef's to let people know that the way that configuration works changed instead of silently supporting both. In the future you probably only want to support one method.

mpack/src/mpack/mpack-platform.h

Lines 51 to 86 in a3fd12f

	#if defined(MPACK_READER) && !MPACK_READER
	#undef MPACK_READER
	#endif
	#if defined(MPACK_EXPECT) && !MPACK_EXPECT
	#undef MPACK_EXPECT
	#endif
	#if defined(MPACK_NODE) && !MPACK_NODE
	#undef MPACK_NODE
	#endif
	#if defined(MPACK_WRITER) && !MPACK_WRITER
	#undef MPACK_WRITER
	#endif
	#if defined(MPACK_STDLIB) && !MPACK_STDLIB
	#undef MPACK_STDLIB
	#endif
	#if defined(MPACK_STDIO) && !MPACK_STDIO
	#undef MPACK_STDIO
	#endif
	#if defined(MPACK_SETJMP) && !MPACK_SETJMP
	#undef MPACK_SETJMP
	#endif
	#if defined(MPACK_DEBUG) && !MPACK_DEBUG
	#undef MPACK_DEBUG
	#endif
	#if defined(MPACK_CUSTOM_ASSERT) && !MPACK_CUSTOM_ASSERT
	#undef MPACK_CUSTOM_ASSERT
	#endif
	#if defined(MPACK_READ_TRACKING) && !MPACK_READ_TRACKING
	#undef MPACK_READ_TRACKING
	#endif
	#if defined(MPACK_WRITE_TRACKING) && !MPACK_WRITE_TRACKING
	#undef MPACK_WRITE_TRACKING
	#endif
	#if defined(MPACK_NO_TRACKING) && !MPACK_NO_TRACKING
	#undef MPACK_NO_TRACKING
	#endif

:)

[ludocode]

I'm actually thinking of switching over to a fourth option, which is that instead of undefining any options that are 0 in mpack-platform.h, it should instead define any undefined ones to 0. This way we can go back to using #if instead of #ifdef everywhere.

This is probably the least error-prone option (or at least it would have been before we switched over to #ifdef, since switching back might be messy.) We avoid #ifdef as much as possible, and if we forget to add a new configuration option to mpack-platform.h or use one before they get defined to 0, we get a nice warning due to -Wundef. This is in contrast to the current situation where if we forget to undefine a new configuration option in mpack-platform.h or #ifdef one before mpack-platform.h, we just get the wrong result.

[ludocode]

Switched over to defining to 0 any undefined options. This way we support -Wundef, we use #if for all configuration switches, and if we forget an option, we get compiler errors instead of silent failures.

mpack/src/mpack/mpack-platform.h

Lines 48 to 105 in 8494718

	/*
	* Now that the config is included, we define to 0 any of the configuration
	* options and other switches that aren't defined. This supports -Wundef
	* without us having to write "#if defined(X) && X" everywhere (and while
	* allowing configs to be pre-defined to 0.)
	*/
	#ifndef MPACK_READER
	#define MPACK_READER 0
	#endif
	#ifndef MPACK_EXPECT
	#define MPACK_EXPECT 0
	#endif
	#ifndef MPACK_NODE
	#define MPACK_NODE 0
	#endif
	#ifndef MPACK_WRITER
	#define MPACK_WRITER 0
	#endif
	#ifndef MPACK_STDLIB
	#define MPACK_STDLIB 0
	#endif
	#ifndef MPACK_STDIO
	#define MPACK_STDIO 0
	#endif
	#ifndef MPACK_DEBUG
	#define MPACK_DEBUG 0
	#endif
	#ifndef MPACK_CUSTOM_ASSERT
	#define MPACK_CUSTOM_ASSERT 0
	#endif
	#ifndef MPACK_READ_TRACKING
	#define MPACK_READ_TRACKING 0
	#endif
	#ifndef MPACK_WRITE_TRACKING
	#define MPACK_WRITE_TRACKING 0
	#endif
	#ifndef MPACK_NO_TRACKING
	#define MPACK_NO_TRACKING 0
	#endif
	#ifndef MPACK_OPTIMIZE_FOR_SIZE
	#define MPACK_OPTIMIZE_FOR_SIZE 0
	#endif
	#ifndef MPACK_EMIT_INLINE_DEFS
	#define MPACK_EMIT_INLINE_DEFS 0
	#endif
	#ifndef MPACK_AMALGAMATED
	#define MPACK_AMALGAMATED 0
	#endif
	#ifndef MPACK_INTERNAL
	#define MPACK_INTERNAL 0
	#endif
	#ifndef MPACK_NO_PRINT
	#define MPACK_NO_PRINT 0
	#endif

This is also much closer to the original code, since 8494718 reverts much of a3fd12f. There are no behavioural changes between versions of MPack, so there shouldn't be any necessity for #warnings to be added. Hopefully we've converged on the best solution!