Stockfish icon indicating copy to clipboard operation
Stockfish copied to clipboard

Fixed 'possible loss of data' warnings

Open maximmasiutin opened this issue 1 year ago • 6 comments

Rationale for change

These code changes address places where variables of higher size are assigning to variables of lower size, which generate compiler warning as a potential place for loss of data when a source value is higher than the target variable can hold. The warnings are given by MSVC (Visual Studio 2019 and 2022) with default compile options.

List of changes

On src/misc.cpp - the sqrtl() (for "long double") is not needed since the result is stored to just "double"; otherwise r should have been defined as "long double", anyway, this does not make any difference in the debug output.

On src/search.cpp - the complexityAverage.value() returns data of type "int64_t", therefore, the variable should also be of type "int64_t", it is later stored into a variable of type "double" anyway after a division, so should not be of any problem.

On src/syzygy/tbprobe.cpp - we first cast unsigned size_t "base64.size()" to signed int "base64_size", and then we are able to subtract 2 to avoid unsigned overflow. Then we use consistent type of the for loop variable, i.e. "int" everywhere.

Fishtest results

https://tests.stockfishchess.org/tests/view/640e1d6165775d3b539cc95d

LLR: -0.12 (-2.94,2.94) <0.00,2.00>
Total: 1000 W: 265 L: 274 D: 461
Ptnml(0-2): 3, 112, 278, 105, 2

maximmasiutin avatar Mar 12 '23 18:03 maximmasiutin

We use type(expr) format for primitive type castings. static_cast seems not necessary.

MinetaS avatar Mar 24 '23 04:03 MinetaS

@MinetaS The compiler gave warning on

uint32_t k = uint32_t(idx / d->span);

Therefore I changed it to

uint32_t k = static_cast<uint32_t>(idx / d->span);

maximmasiutin avatar Mar 24 '23 17:03 maximmasiutin

Could you show me what the warning message is? We are already using such convention across the entire source code.

MinetaS avatar Mar 24 '23 17:03 MinetaS

@MinetaS - here they are:

\src\misc.cpp(357,53): warning C5219: implicit conversion from 'int64_t' to 'double', possible loss of data
\src\misc.cpp(378,22): warning C4244: 'initializing': conversion from 'long double' to 'double', possible loss of data
\src\misc.cpp(387,22): warning C4244: 'initializing': conversion from 'long double' to 'double', possible loss of data
\src\search.cpp(235,44): warning C4244: 'argument': conversion from 'double' to 'int', possible loss of data
\src\search.cpp(305,36): warning C4244: 'argument': conversion from 'double' to 'int', possible loss of data
\src\search.cpp(475,26): warning C4244: 'initializing': conversion from 'int64_t' to 'int', possible loss of data
\src\syzygy\tbprobe.cpp(544,43): warning C4267: 'initializing': conversion from 'size_t' to 'int', possible loss of data
\src\syzygy\tbprobe.cpp(1004,5): warning C4267: 'initializing': conversion from 'size_t' to 'int', possible loss of data

maximmasiutin avatar Mar 31 '23 23:03 maximmasiutin

There are the following warnings in the current (today's) master in this file. The MSVC2022 is set with default settings.

src\syzygy\tbprobe.cpp(544,43): warning C4267: 'initializing': conversion from 'size_t' to 'int', possible loss of data
src\syzygy\tbprobe.cpp(1004,5): warning C4267: 'initializing': conversion from 'size_t' to 'int', possible loss of data
src\thread.cpp(202,68): warning C4244: '=': conversion from '_Ty' to 'int', possible loss of data

maximmasiutin avatar Apr 01 '23 16:04 maximmasiutin

Ok, so this is not a problem of the syntax but rather a compiler issue (MSVC).

The first snippet of code converts uint64_t to uint32_t implicitly, and the second one has explicit conversion operator.

int main() {
    uint64_t x;
    std::cin >> x;
    uint32_t y = x;
    std::cout << y << std::endl;
    return 0;
}
int main() {
    uint64_t x;
    std::cin >> x;
    uint32_t y = uint32_t(x);
    std::cout << y << std::endl;
    return 0;
}

...and gcc/clang command line is:

g++ test.cc -Wconversion -o test

The first version of code leads to a warning with both compilers.

gcc:

test.cc: In function ‘int main()’:
test.cc:7:18: warning: conversion from ‘uint64_t’ {aka ‘long unsigned int’} to ‘uint32_t’ {aka ‘unsigned int’} may change value [-Wconversion]
    7 |     uint32_t y = x;
      |

clang:

test.cc:7:18: warning: implicit conversion loses integer precision: 'uint64_t' (aka 'unsigned long') to 'uint32_t' (aka 'unsigned int') [-Wshorten-64-to-32]
    uint32_t y = x;
             ~   ^

But the second version has no warnings. This means that programmer showed their intention explicitly in the code, and both compilers consider it understandable. From my perspective, MSVC complaining of not using static_cast about simple primitive conversions is quite hysteric. (not to mention it obviously deteriorates readability)

So, I believe suppressing warnings in this way is not the best option we can have.

Other changes not involving static_cast look good to me. Please consider submitting a new PR or updating this one.

MinetaS avatar Apr 08 '23 14:04 MinetaS

partially merged via https://github.com/official-stockfish/Stockfish/commit/a9a0dbbcd0749b4e6255c7e9a17f19cffedaa531, thanks!

snicolet avatar Aug 22 '23 11:08 snicolet