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.

[t-b]

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.

Globally disabling?

[nlohmann]

I'm not a big fan of that - especially since it seems not to be enabled by default or is it?

[gregmarr]

Since the example shown is in a test file, I think it would be acceptable to have a pragma in that file to disable the warning. If it were in the main source, that would be a different story.

[nlohmann]

diff --git a/test/src/unit-json_pointer.cpp b/test/src/unit-json_pointer.cpp
index 14d8cd183..fc10c6ff5 100644
--- a/test/src/unit-json_pointer.cpp
+++ b/test/src/unit-json_pointer.cpp
@@ -358,6 +358,10 @@ TEST_CASE("JSON pointers")
                 CHECK_THROWS_WITH(j_const[jp] == 1, throw_msg.c_str());
             }
 
+#if defined(_MSC_VER)
+#pragma warning (push)
+#pragma warning (disable : 4127) // on some machines, the check below is not constant
+#endif
             if (sizeof(typename json::size_type) < sizeof(unsigned long long))
             {
                 auto size_type_max_uul = static_cast<unsigned long long>((std::numeric_limits<json::size_type>::max)());
@@ -371,6 +375,10 @@ TEST_CASE("JSON pointers")
                 CHECK_THROWS_WITH(j_const[jp] == 1, throw_msg.c_str());
             }
 
+#if defined(_MSC_VER)
+#pragma warning (pop)
+#endif
+
             CHECK_THROWS_AS(j.at("/one"_json_pointer) = 1, json::parse_error&);
             CHECK_THROWS_WITH(j.at("/one"_json_pointer) = 1,
                               "[json.exception.parse_error.109] parse error: array index 'one' is not a number");

@gregmarr @t-b @Andreas-Schniertshauer Is this what you'd have in mind?