search

meganz / mingw-std-threads

Standard threads implementation currently still missing on MinGW GCC on Windows
BSD 2-Clause "Simplified" License
439 stars 137 forks source link

Silencing warnings to do with using old style casts and also those ar… #46

Closed nacitar closed 5 years ago

nacitar commented 5 years ago

…ising

from integral promotion of smaller types.

alxvasilev commented 5 years ago

HI, thanks for the MR. I'd like to clarify if we really need all of them, more specifically these casts: static_cast<counter_type>(expected + 1)? @nmcclatchey Have you observed any warnings other then the C-style casts?

nacitar commented 5 years ago

The C-Style cast comes from -Wold-style-cast, which some people (like me) enable... and I also enable -Werror =P

The other casts on the arithmetic result are indeed necessary because when counter_type is smaller than int (like a short), the result of expected + 1 yields an "int" rather than the original "counter_type" (short, in this example). When you assign the result back/pass it to a counter_type argument, you're now truncating an "int" into a "short" which gives you the warning about losing information.

nacitar commented 5 years ago

EXAMPLE: error: conversion to ‘std::__atomic_base::__int_type {aka short unsigned int}’ from ‘int’ may alter its value [-Werror=conversion]

alxvasilev commented 5 years ago

I see, makes sense. But then what about: https://github.com/meganz/mingw-std-threads/blob/c68a2e6bfe56a62f35cdc7a53a91c525731d6cac/mingw.shared_mutex.h#L103 kWriteBit is also of counter_type.

nacitar commented 5 years ago

I see, makes sense. But then what about: if (expected >= kWriteBit - 1). kWriteBit is also of counter_type.

In that case you aren't taking that temporary (int) and trying to store it back inside a (counter_type), you're just comparing that temporary to a counter_type, which is fine. =)

alxvasilev commented 5 years ago

Ok, so you confirm there is no warning when expected gets extended to int. All ok then.

alxvasilev commented 5 years ago

Merged, thank you!

nmcclatchey commented 5 years ago

@alxvasilev I hadn't observed any warnings related to this, but that's probably due to missing a warning flag, or using a different combination of macro definitions (-Wconversion wasn't enabled by my flavor of warning flags, for example). It's good that it came up, though. I can see several bugs related to type conversion that need to be fixed (not just have the warnings silenced). I'll have a PR ready soon with those fixes in place.

@nacitar Silencing the warnings is good for usability (because of -Werror), but the warnings (usually) exist for a reason; we can use them to improve the code instead. The warnings can help to find true failure cases, such as when a timeout would overflow during conversion (or, in some cases, be the special Windows API value INFINITE, which is it's own kind of trouble).