fix warnings in serializer.hpp for VS2019

[dota17]

Refer to #1911
fix warnings in serializer.hpp for VS2019

[coveralls]

Coverage remained the same at 100.0% when pulling 211b8b8 on dota17:dota17-warning into 973c52d on nlohmann:develop.

[nlohmann]

I have some minor comments.

[MBalszun]

As byte is unsigned, the check whether it is >= 0 is redundant, right?

@nlohmann , @dota17:

byte < 400 is redundant too, because a std::uint8_t can be at most 255.

If anything, the expression 256 + (std::size_t)state * 16u + (std::size_t)type should be bounds checked. Honestly: Can't we just use the original code and suppress the warning from the msvc analyzer? Those casts add absolutely nothing in terms of correctness, readability or safety to the code.

What MSVC complains about is that computation is done in 32 bits first and then the result is widened to 64bits (a.k.a. std::size_t). Such a warning makes sense in circumstances where people expect the result of a computation might need more than 32 bits and thus assign the result to a 64 bit variable, but forget that the computation itself (and resulting overflow) still happens in 32 bits.

But here the result must be less than 400 (the size of the array) anyway and it doesn't matter if computation happens in 32 or 64 bits. At most, an argument can be made that computation has to happen in something bigger than 8 bits, but this is anyway happening as per the language rules no matter the input types (any small integral type gets promoted to int or unsigned int before the actual operation takes place).

[dota17]

If anything, the expression 256 + (std::size_t)state * 16u + (std::size_t)type should be bounds checked. Honestly: Can't we just use the original code and suppress the warning from the msvc analyzer? Those casts add absolutely nothing in terms of correctness, readability or safety to the code.
What MSVC complains about is that computation is done in 32 bits first and then the result is widened to 64bits (a.k.a. std::size_t). Such a warning makes sense in circumstances where people expect the result of a computation might need more than 32 bits and thus assign the result to a 64 bit variable, but forget that the computation itself (and resulting overflow) still happens in 32 bits.

I agree with you.
What I worry about is that maybe it is not suitable for shutting up a compiler warning for a single line. But if people think it is ok, everything will be fine.

In addition, @nlohmann , what is this project's policy about these annoying compiler warnings?

[nlohmann]

The "policy" of the library is to be warning-free where possible. However, this is getting more and more difficult, because some warnings are loaded with stylistic opinions, and it gets impossible to write code where all compilers remain silent.

In this specific case, it seems only MSVC complains, and though it would be nice to silence the warning, I am hesitant to achieve this "whatever it takes".

I put the decode function into C++ Insights which translates

codep = (state != UTF8_ACCEPT)
        ? (byte & 0x3fu) | (codep << 6u)
        : (0xFFu >> type) & (byte);

state = utf8d[256u + state * 16u + type];

into

codep = (static_cast<int>(state) != static_cast<int>(UTF8_ACCEPT))
      ? (static_cast<unsigned int>(byte) & 63U) | (codep << 6U)
      : (255U >> static_cast<int>(type)) & static_cast<unsigned int>((byte));

state = utf8d.operator[](static_cast<unsigned long>((256U + (static_cast<unsigned int>(state) * 16U)) + static_cast<unsigned int>(type)));

Maybe this can help to find a fix for the issue.

[dota17]

As byte is unsigned, the check whether it is >= 0 is redundant, right?

Right.
From #1911 , it said that Warning C28020 The expression '0<=_Param_(1)&&_Param_(1)<=400-1' is not true at this call.. So i added this.
And from #859, it may be a false positive.

The newest version of this PR had move it out.

[dota17]

There are some errors in CI. And i tried to fix it.
However, the errors in #2004, #2002 are same as this issue, especially #2004 just add a yml file and don't change any code.

[dota17]

Ready to go.

[nlohmann]

Looks good to me.

[nlohmann]

Thanks!

[nlohmann]

---

🔖 Release item

This issue/PR will be part of the next release of the library. This template helps preparing the release notes.

Type

• ✨ New Feature
• 🐛 Bug Fix
• ⚡️ Improvement
• 🔨 Further Change
• 🔥 Deprecated function

Description

• Fix a compiler warning in MSVC. fix warnings in serializer.hpp for VS2019 #1969 Compile warnings on MSVC 14.2 #1911

---