Modernize integer comparison

[codenut]

Replace static_cast<size_t>(-1) with std::numeric_limitsstd::size_t::max() as suggested in #4559

Builds are failing in MSVS because of <Windows.h> header file that includes macro definitions named max and min:

#define max(a,b) (((a) > (b)) ? (a) : (b))

one way to solve this issue is to wrap max with parenthesis.

Source: https://stackoverflow.com/questions/27442885/syntax-error-with-stdnumeric-limitsmax

[coveralls]

coverage: 99.639%. remained the same
when pulling 182fa40 on codenut:develop
into 2134cb9 on nlohmann:develop.

[nlohmann]

Builds are failing in MSVS because of <Windows.h> header file that includes macro definitions named max and min:

#define max(a,b) (((a) > (b)) ? (a) : (b))

one way to solve this issue is to wrap max with parenthesis.

Source: stackoverflow.com/questions/27442885/syntax-error-with-stdnumeric-limitsmax

Thanks, this is the way we do it in the other parts of the library. The failing AppVeyor test was explicitly added to detect these problems.

[nlohmann]

Looking at the diff, it becomes obvious how frequent we use -1 as magic number for "unknown size". As it is mostly used in the context of the SAX interface, it could be a good idea to define a constant unknown_size in that header and use this throughout the code.

[nlohmann]

Note this PR only addresses one aspect of #4559 - the bigger part is about using cmp_not_equal and the likes as #4558.

[codenut]

Note this PR only addresses one aspect of #4559 - the bigger part is about using cmp_not_equal and the likes as #4558.

Thank you for reviewing my PR. Would it be possible to address your feedback in a separate PR? I will create a new PR and issue for that purpose.

Edit: Sorry, I misunderstood your comment. I will make the changes as suggested by @gregmarr

[codenut]

Note this PR only addresses one aspect of #4559 - the bigger part is about using cmp_not_equal and the likes as #4558.

Do you think implementing the comparison helper functions is a "good first issue"? If it is, I am interested in working on that too.

[gregmarr]

As it is mostly used in the context of the SAX interface, it could be a good idea to define a constant unknown_size in that header and use this throughout the code.

It's used in the parser, and in the main class in the set_parent function, so I don't think burying it in the SAX interface is a good idea. Also, having it as a constant, even a constexpr constant, is potentially opening up storage and linkage issues, so it's probably better to just have it as a constexpr function in the detail namespace.

NLOHMANN_JSON_NAMESPACE_BEGIN
namespace detail
{
constexpr std::size_t unknown_size() { return (std::numeric_limits<std::size_t>::max)(); }
////////////
// parser //
////////////

[gregmarr]

Note this PR only addresses one aspect of #4559 - the bigger part is about using cmp_not_equal and the likes as #4558.

Do you think implementing the comparison helper functions is a "good first issue"? If it is, I am interested in working on that too.

It depends. If by "first issue" you mean first in this repository, but you have significant experience in template metaprogramming and SFINAE, then maybe. If you mean a good issue for an inexperienced programmer, then no.

The cmp_not_equal function requires C++20 or later. This would require writing a helper function for C++11 through C++17. The "possible implementation" of cmp_not_equal shown at cppreference.com uses if constexpr, which is only available in C++17 and later. So there would be one version with if constexpr for C++17, and one set of functions with SFINAE for C++11 and C++14.

However, I think that using max instead of std::size_t(-1) will actually eliminate the need for cmp_not_equal, as that was a way to eliminate the warnings with the signed and unsigned comparisons, which will no longer happen because we're using an alternate method to get the value.

[nlohmann]

If this PR is sufficient to fix the Clang-Tidy warning modernize-use-integer-sign-comparison´, then I am more than happy to leave the cmp_not_equal` alone.

[nlohmann]

As it is mostly used in the context of the SAX interface, it could be a good idea to define a constant unknown_size in that header and use this throughout the code.

It's used in the parser, and in the main class in the set_parent function, so I don't think burying it in the SAX interface is a good idea. Also, having it as a constant, even a constexpr constant, is potentially opening up storage and linkage issues, so it's probably better to just have it as a constexpr function in the detail namespace.

NLOHMANN_JSON_NAMESPACE_BEGIN
namespace detail
{
constexpr std::size_t unknown_size() { return (std::numeric_limits<std::size_t>::max)(); }
////////////
// parser //
////////////

set_parent() is never called with the size_t(-1) argument explicitly. It is the default argument. So it would be sufficient to change line

reference set_parent(reference j, std::size_t old_capacity = static_cast<std::size_t>(-1))

to

reference set_parent(reference j, std::size_t old_capacity = std::numeric_limits<std::size_t>::max()))

with a respective comment about this value.

Then we have the following appearances of static_cast<std::size_t>(-1):

• binary_reader.hpp: in get_cbor_array and get_cbor_object
• binary_reader.hpp: calling the SAX interface for start_array/start_object
• json_sax.hpp: checking the len value in start_array/start_object
• json_sax.hpp: default parameter in json_sax_acceptor
• parser.hpp: calling the SAX interface for start_array/start_object

Interestingly, we also have in binary_reader:

static JSON_INLINE_VARIABLE constexpr std::size_t npos = static_cast<std::size_t>(-1);

which is used at some places.

---

While this comment should make the case for moving the value closer to the parser, I think I over-engineered this in my head. So I think having a detail::unknown_size() function is sufficient to get rid of the magic values.

[nlohmann]

Looks good to me.

[nlohmann]

Thanks!

[codenut]

Thanks!

Thank you for your thorough feedback. I look forward to collaborating on more "first good issue" changes!

[nlohmann]

You could help by leaving a review at #4581.