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

[mbeutel]

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

[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.

[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.