Tests fail to compile due to -Werror=type-limits when char is unsigned

[musicinmybrain]

On Fedora Linux Rawhide, with GCC 13.0.1, while testing gsl-lite 0.41.0, we’re seeing that test/lest_cpp03.hpp fails to compile with an error from -Werror=type-limits on our non-x86 architectures (aarch64, ppc64le, s390x).

In file included from /builddir/build/BUILD/gsl-lite-0.41.0/test/gsl-lite.t.hpp:104,
                 from /builddir/build/BUILD/gsl-lite-0.41.0/test/emulation.t.cpp:20:
/builddir/build/BUILD/gsl-lite-0.41.0/test/lest_cpp03.hpp: In function 'bool lest::unprintable(char)':
/builddir/build/BUILD/gsl-lite-0.41.0/test/lest_cpp03.hpp:707:46: error: comparison is always true due to limited range of data type [-Werror=type-limits]
  707 | inline bool unprintable( char c ) { return 0 <= c && c < ' '; }
      |                                            ~~^~~~

This seems to be happening because char is equivalent to signed char on i686 and x86_64, but is equivalent to unsigned char on the other platforms. The useless comparison is not a real problem here, but the failure to compile is.

I am not sure why we are seeing this in 0.41.0 but not in 0.40.0. Nothing stands out in the changes as potentially relevant.

There is existing logic to suppress this warning on “GNU” compilers, but only for old compiler versions:

gsl-lite/test/MakeTestTarget.cmake

Lines 264 to 266 in 755ba12

	if( CMAKE_CXX_COMPILER_VERSION VERSION_LESS 4.8 )
	list( APPEND localOptions "-Wno-type-limits" ) # irrelevant warning about `unsigned value < 0` comparison
	endif()

Should we just make that unconditional, like -Wno-long-long and -Wuseless-cast?

[mbeutel]

Thank you for the report. The problem might be related to this change to the PCH setup. Previously, we had simply set gsl-lite.t.hpp as the precompiled header; but apparently this didn't interact well with the warning suppressions in that header file; at least I had to address several previously unseen warnings after making these changes.

As a general rule, I would like to avoid unconditional warning suppressions for current compiler versions. "-Wno-long-long", being relevant only for C++98, is an exception; "-Wuseless-cast" unconditionally enables the warning which is fine.

(Edit: pardon me, I had only looked at MakeTestTarget.cmake. We indeed have a lot of unconditional warning suppressions in gsl-lite.t.hpp, "-Wuseless-cast" being among them. As a comment there points out, this is required to pacify the compiler with regard to lest. I aim to eventually replace our outdated and somewhat customized copy of lest with Catch2.)

If the warning appears only once, I'd prefer to work around locally, either by suppressing it:

inline bool unprintable( char c )
{
#if defined (__GNUC__)
# pragma GCC diagnostic suppress "-Wtype-limits"  // comparison is always true due to limited range of data type
#endif // defined (__GNUC__)
    return 0 <= c && c < ' ';
}

or by casting c to signed char:

inline bool unprintable( char c ) { return 0 <= static_cast<signed char>(c) && c < ' '; }

Would you mind verifying if either of these fixes the issue, and possibly issuing a PR?

Edit: you can also add "-Wtype-limits" to the ignored diagnostics in gsl-lite.t.hpp if you like. I'll find it there if I ever get around to replacing lest.

[martinmoene]

An intermediate step could perhaps be to suppress lest's warnings by handling it as a system header. See e.g. this commit of string_view lite.

[musicinmybrain]

Thank you for the report. The problem might be related to this change to the PCH setup. Previously, we had simply set gsl-lite.t.hpp as the precompiled header; but apparently this didn't interact well with the warning suppressions in that header file; at least I had to address several previously unseen warnings after making these changes.

Thanks for explaining that; it seems like a plausible root cause.

As a general rule, I would like to avoid unconditional warning suppressions for current compiler versions. […]

If the warning appears only once, I'd prefer to work around locally, either by suppressing it:

[…]

or by casting c to signed char:

[…]

Would you mind verifying if either of these fixes the issue, and possibly issuing a PR?

Thanks for the suggestions. Both of these look reasonable; if you have no particular preference, I think I prefer the cast. I’ve tested it as a patch and opened a PR.

[mbeutel]

Thanks for the contribution. Do you need a patch release (v0.41.1) as soon as possible? If not, I'd let it sit for a few days before issuing a release in case there is more fallout.

[mbeutel]

@martinmoene, thanks for the suggestion, I think that might indeed be better than suppressing all these warnings in all of our test suite.

[musicinmybrain]

Thanks for the contribution. Do you need a patch release (v0.41.1) as soon as possible? If not, I'd let it sit for a few days before issuing a release in case there is more fallout.

There is no hurry at all. Thanks for working on this.