decode(state, codep, byte) generates warnings.

[SimonPeacock]

Description

Quite minor.. BUT

json.hpp, line 18822 .. 1/ the third parameter is named byte, 'byte' is also a 'type' and may lead to confusion. It is also a public variable used in parse_error class which isn't an 8-bit value (json.hpp, line 4440)

2/ JSON_ASSERT(byte < utf8d.size()); (json.hpp, line 18844) Since byte is declared as an uint8_t, its range is 0 < byte < 256. This means the assert on an array of size 400 will never fail. VS2019 sometimes produces a warning for this.

3/ JSON_ASSERT(index < 400); (json.hpp, line 18852) Shouldn't this be JSON_ASSERT(index < utf8d.size());

Simon

Reproduction steps

"Build"->"Run Code Analyzer on Solution"

Expected vs. actual results

n/a

Minimal code example

No response

Error messages

Warning	C28020	The expression '0<=_Param_(1)&&_Param_(1)<=400-1' is not true at this call.

This message doesn't always show up, I ended up analyzing the code ("Build"->"Run Code Analyzer on Solution")

Compiler and operating system

Windows 10, Microsoft Visual Studio Professional 2019

Library version

version 3.10.4

Validation

• [X] The bug also occurs if the latest version from the develop branch is used.
• [ ] I can successfully compile and run the unit tests.

[falbrechtskirchinger]

json.hpp, line 18822 .. 1/ the third parameter is named byte, 'byte' is also a 'type' and may lead to confusion. It is also a public variable used in parse_error class which isn't an 8-bit value (json.hpp, line 4440)

I'm not too concerned about confusion with std::byte, especially since decode() is an internal function. We can't change a public member variable name until 4.0.0.

2/ JSON_ASSERT(byte < utf8d.size()); (json.hpp, line 18844) Since byte is declared as an uint8_t, its range is 0 < byte < 256. This means the assert on an array of size 400 will never fail. VS2019 sometimes produces a warning for this.

I'd be OK with either re-writing or completely removing the assertion.

3/ JSON_ASSERT(index < 400); (json.hpp, line 18852) Shouldn't this be JSON_ASSERT(index < utf8d.size());

Yes.

[gregmarr]

It is also a public variable used in parse_error class which isn't an 8-bit value (json.hpp, line 4440)

That's fine. byte by itself is not a reserved word. It is referring to the particular byte in the stream, so it is a perfectly reasonable name. We could change it to byte_index but I don't think that's necessary.

[nlohmann]

My 2 cents:

1. The name byte makes sense here: the variable actually is a byte; this name is also used on http://bjoern.hoehrmann.de/utf-8/decoder/dfa/ where we got the code from. I would leave it as is. As @gregmarr pointed out, we can't rename detail::parse_error::byte as it would be a breaking change.
2. I'd like to leave the assertion there, because it documents that we are on the safe side to access the array at that index. Removing the assertion because it just cannot happen would then yield a comment that basically states the same: it is safe to use that untrusted value as array index. I'd rather have code than comments here.
3. Right - 400 is an unnecessary magic number here, and utf8d.size() much more expressive.

I'll create a merge request.