Warnings & Errors in various configurations

[helmesjo]

Since I'm maintaining the build2 package of glaze I just ran it through the (free) CI servers and figured I'll let you know which has warnings & which has errors (in case you'd rather just tackle them in one go).

See build status here for v2.6.8.

Edit.
Obviously you can ignore the warnings coming from Eigen headers.
I should also mention that there appears to be a sporadic error in the jsonrpc_test on Windows (unless it's something wrong specifically with my build2 package).

[stephenberry]

Thanks for sharing. Most of these warnings are from Eigen or asio, but I did spot one potential under-read issue for glaze flags, which I'll look into.

Could you point me to the cases of the sporadic error in jsonrpc_test on Windows?

Glaze also runs CI on a bunch of platforms and configurations under its GitHub Actions. But, your CI tests show that I should probably also be build testing with the c++26 flag where possible.

[helmesjo]

Yep, and especially MSVC complains about a few things. One is the use of <expected> with #if __has_include(<expected>) which should instead be checked something like this:

#ifdef __has_include
# if __has_include(<version>)
#   include <version> // bring in feature-macros
# endif
#endif

#if __cpp_expected >= 202202L
#  include <expected>
#endif

Could you point me to the cases of the sporadic error in jsonrpc_test on Windows?

That would be jsonrpc_test.cpp:43 with test_name = response:{"jsonrpc":"2.0","result":6,"id":1}. But I can see that it crashes inside ut.hpp:1674:

ss.str() is not popping because '@

Edit.
This is with boost-ut v2.0.1.
Also, this test should have exceptions turned off (I think), but it just hits std::abort() in that case.

[stephenberry]

So, I've tried multiple times to fix the MSVC warning for <expected>, but older versions of Clang break with these kinds of checks. If you're able to make a pull request that passes, that would be great, but otherwise we've just been living with these warnings and they'll go away in C++23 and beyond.

Yeah, boost-ut has some major issues. I've been trying to get them to fix some random segfaults from simply including the header. I've developed a simpler ut library that matches the core API of boost-ut that I will probably switch over to.

[helmesjo]

What is the oldest Clang version you support?

[helmesjo]

And just since I had them pop up right now (I build with -W4):

glaze\json\read.hpp(2287) : warning C4702: unreachable code
glaze\json\read.hpp(2289) : warning C4702: unreachable code
glaze\json\read.hpp(2292) : warning C4702: unreachable code
glaze\json\read.hpp(2293) : warning C4702: unreachable code
glaze\json\read.hpp(2296) : warning C4702: unreachable code
glaze\json\read.hpp(2297) : warning C4702: unreachable code
glaze\json\read.hpp(2308) : warning C4702: unreachable code
glaze\json\read.hpp(2312) : warning C4702: unreachable code
glaze\json\read.hpp(2315) : warning C4702: unreachable code

[stephenberry]

Clang 15 should build. I run actions with 16, 17, & 18

[stephenberry]

Unreachable code is annoying to deal with in Glaze because it often means needing to duplicate code (because of so many if constexpr branches), and the compiler will typically simply remove unreachable code. I'm wondering if I should just disable these warnings locally?

[helmesjo]

You could probably just be explicit with std::unreachable().
That is, define a macro GLZ_UNREACHABLE if #if defined(__cpp_lib_unreachable) && __cpp_lib_unreachable >= 202202L.

[stephenberry]

Yeah, Glaze uses a bit of unreachable. The problem in these cases is that the code is conditionally unreachable. I can use an if constexpr check to throw in an unreachable when that would be the case, but the compiler will still complain that the rest of the code is unreachable, so std::unreachable doesn't actually improve anything.

This is typically a problem with constexpr conditional tail/end behavior.

[helmesjo]

Ok I see. Well, I'll just disable that warning (/w4702) for the tests in my build2 package. Don't think I've seen it in my own project consuming glaze.

[stephenberry]

I'll take another look at whether I should refactor the code or just locally disable the warning. Thanks for pointing these out.

[helmesjo]

While we're at it with MSVC (unless you already saw this warning):
read.hpp(36): warning C4244: 'argument': conversion from 'const int' to 'const uint8_t', possible loss of data
when compiling tests\binary_test\binary_test.cpp(1284) (probably a few others as well).

[stephenberry]

Yeah, I saw those. Working on them now.

[stephenberry]

To explain a bit more why avoiding unreachable is a challenge:
If Glaze were to use exceptions we could easily make local lambda functions that wrap up behavior and then just call these functions in the appropriate if constexpr branches. However, because Glaze does not use exceptions internally any lambda function breaks our ability to return from the function at hand, we just return from the lambda. This requires an additional error check after the lambda is called, to determine if an error occurred in the lambda, instead of just exiting from the function. So, we lose performance by using lambdas.

This is a problem that requires deterministic and non-allocating exception support in C++. There have been proposals to improve exception handling in C++, but I think the broader community needs to see this as more of a critical issue.

[stephenberry]

I just merged in #1039, which removes most the warnings across the compilers.

[stephenberry]

I'm moving to a new version of ut to avoid issues in the library, which should solve the problem seen in the jsonrpc tests. In #1041