Warnings C4715 and C4127 when building json-3.9.1 with Visual Studio 2019 16.7.7

[Andreas-Schniertshauer]

I get the following warnings when building json-3.9.1 with Visual Studio 2019 16.7.7:

single_include\nlohmann\json.hpp(4697): warning C4715: 'nlohmann::detail::hash<nlohmann::basic_json<std::map,std::vector,std::basic_string<char,std::char_traits<char>,std::allocator<char> >,bool,__int64,unsigned __int64,double,std::al
locator,nlohmann::adl_serializer,std::vector<unsigned char,std::allocator<unsigned char> > > >': not all control paths return a value

test\src\unit-json_pointer.cpp(362,78): warning C4127: conditional expression is constant

Please describe the steps to reproduce the issue.

1. Download json-3.9.1.zip and unzip
2. Open a Developer command Prompt v16.7.7
3. cd json-3.9.1
4. Create directory build and cd build
5. cmake ..\ -D JSON_Install:BOOL=OFF -D JSON_ImplicitConversions:BOOL=OFF -D JSON_BuildTests:BOOL=ON
6. cmake --build .

What is the expected behavior?

No warnings

And what is the actual behavior instead?

Warnings

Which compiler and operating system are you using?

• Compiler: Visual Studio 2019 16.7.7 Professional
• Operating system: Windows 10 2004 Pro German

Which version of the library did you use?

• latest release version 3.9.1

[nlohmann]

Warning

single_include\nlohmann\json.hpp(4697): warning C4715: 'nlohmann::detail::hash<nlohmann::basic_json<std::map,std::vector,std::basic_string<char,std::char_traits<char>,std::allocator<char> >,bool,__int64,unsigned __int64,double,std::al
locator,nlohmann::adl_serializer,std::vector<unsigned char,std::allocator<unsigned char> > > >': not all control paths return a value

seems to be a false positive, because function hash(const BasicJsonType& j) uses a an exhaustive switch statement where all reachable branches return a value. The default branch consists of an assert(false) call which, if it would be reachable, does not leave the function, but abort.

Warning

test\src\unit-json_pointer.cpp(362,78): warning C4127: conditional expression is constant

seems to be a false positive. json::size_type is std::size_t whose size is implementation-defined, but at least 16 bit. unsigned long long is at least 64 bit. So

sizeof(typename json::size_type) < sizeof(unsigned long long)

is true very often, but not necessarily all the time.

[nlohmann]

I'm not sure how to fix any of the warnings.

[Andreas-Schniertshauer]

sizeof(typename json::size_type) < sizeof(unsigned long long)

can be solved in c++17 with

if constexpr (sizeof(typename json::size_type) < sizeof(unsigned long long))

[Andreas-Schniertshauer]

The

default: // LCOV_EXCL_LINE

case does not return anything.

[t-b]

json::size_type is std::size_t whose size is implementation-defined, but at least 16 bit. unsigned long long is at least > 64 bit. So

sizeof(typename json::size_type) < sizeof(unsigned long long)
is true very often, but not necessarily all the time.

MSVC does not warn about the general case but about the case for the architecture being compiled.

[nlohmann]

can be solved in c++17 with

if constexpr (sizeof(typename json::size_type) < sizeof(unsigned long long))

The library targets C++11.

[nlohmann]

The

default: // LCOV_EXCL_LINE

case does not return anything.

That's true, but it also does not leave the function due to the assertion.

[gregmarr]

If there were a return after the assert we'd probably get an unreachable code warning on it on other compilers.

This may help https://stackoverflow.com/questions/60802864/emulating-gccs-builtin-unreachable-in-visual-studio

[gregmarr]

This exact case is covered here: https://docs.microsoft.com/en-us/cpp/intrinsics/assume?view=msvc-160&viewFallbackFrom=vs-2019

[nlohmann]

@gregmarr Thanks for the reference! A NODEFAULT macro could be a good addition to the library. Interestingly, we have several other places where we did put a return after an assertion, like

        default:                   // LCOV_EXCL_LINE
            JSON_ASSERT(false);    // LCOV_EXCL_LINE
            return 0;              // LCOV_EXCL_LINE

[gregmarr]

If we already have places where we return after an assert and we're not getting errors there, then adding a return in this case seems like a reasonable fix.

[nlohmann]

The issue with the missing return value is fixed in the devlop branch: dd8cb2a#diff-a9407ccd45e5c9f004f9d81a18c381d0e5ecd782d5a91ec8f64614d3da6a7eaeR8397

I went through the code and did not find a similar issue. I would close this issue unless anyone has an idea what to do with C4127.