GCC -Wuseless-cast warnings

[tambry]

Using the GCC -Wuseless-cast results in the following warnings:

_deps/json-src/single_include/nlohmann/json.hpp: In member function ‘bool nlohmann::detail::binary_reader<BasicJsonType, SAX>::parse_cbor_internal(bool)’:
_deps/json-src/single_include/nlohmann/json.hpp:4058:109: warning: useless cast to type ‘std::size_t’ {aka ‘long unsigned int’} [-Wuseless-cast]
 4058 |                 return get_number(input_format_t::cbor, len) and get_cbor_array(static_cast<std::size_t>(len));
      |                                                                                                             ^
_deps/json-src/single_include/nlohmann/json.hpp:4112:110: warning: useless cast to type ‘std::size_t’ {aka ‘long unsigned int’} [-Wuseless-cast]
 4112 |                 return get_number(input_format_t::cbor, len) and get_cbor_object(static_cast<std::size_t>(len));
      |                                                                                                              ^

[t-b]

That is actually a bug hiding here:

e.g.

            case 0xBB: // map (eight-byte uint64_t for n follow)
            {
                std::uint64_t len;
                return get_number(input_format_t::cbor, len) and get_cbor_object(static_cast<std::size_t>(len));
            }

that only works if you can fit a 64bit unsigned integer into a size_t.

[nlohmann]

Any idea how to proceed on this?

[tambry]

@nlohmann Change get_cbor_array() and get_cbor_object() to take std::uint64_t. This will prevent an overflow for the 0x9B and 0xBB cases on platforms where sizeof(std::size_t)==sizeof(std::uint32_t). Then you can remove the superfluous casts.

[jaredgrubb]

I dont think there's a bug. The scenario would be that there's a platform that wants to read CBOR that has more than 4 billion things on a platform that has memory buffers that literally cannot hold 4 billion things. That won't work.

I think maybe GCC is being too aggressive here. unsigned long and uint64_t are always distinct types. These types of warnings are meant to flag suspicious or dubious things, and casting between two types of the same size (on the platform reporting this issue) does not feel like a dubious thing to do.

I'm tempted to say this is Behaving Correctly. What do you all think? (A patch could be to pragma-ignore this warning in our header for GCC if we want to be warnings-clean for our users).

[tambry]

The scenario would be that there's a platform that wants to read CBOR that has more than 4 billion things on a platform that has memory buffers that literally cannot hold 4 billion things. That won't work.

Technically possible on x86 with PAE.

That said, couldn't the number of elements be small enough (e.g. 163) despite it being a 64-bit length?

unsigned long and std::uint64_t are always distinct types

Certainly not. std::uint64_t is simply an alias to a type whose size is at least 64-bit. This may well be unsigned long depending on your platform.

I would be OK with simply being clean for this warning though.

[jaredgrubb]

This is a language concept not a platform concept. size_t is defined to be the type that can hold the biggest size (sizeof(a)) or, as a corollary, an index into an array (or buffer). So if size_t is 32-bit, then you literally cannot have buffers that are bigger than 4GB. (This is subtly different from what you're describing is where you can have 32-bit pointers on a machine with 8GB of total physical memory, where the virtual memory subsystem makes it all work out.)

You're right about the uint64_t and size_t; I thought C++ required these to be different (and have different mangling like 'long' has) but you're right.

Still I don't really think there is something to fix here?

[t-b]

Well we could throw an exception if we try to read an array/object with uint64 size and have only a std::size_t < 64.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[nlohmann]

Fixed with #2902.