Codechange: generify ClampTo<type> #10756
Conversation
rubidium42
force-pushed
the
clampto
branch
3 times, most recently
from
d5d6e1c
to
7f07838
Compare
src/core/math_func.hpp
Outdated
| return static_cast<uint16>(std::min(a, static_cast<uint64>(UINT16_MAX))); | ||
| if constexpr (std::is_same<To, From>::value) { | ||
| /* If we clamp to the same type, just do nothing. */ | ||
| return value; |
There was a problem hiding this comment.
I wonder if this should be an error. It seems to me that attempting to Clamp a variable to the same type is always a logic error somewhere, and could be hiding some other problem?
There was a problem hiding this comment.
I guess that probably also holds for cases where sizeof(From) < sizeof(To).
Though I wonder whether it should directly become an error. As this way it allows you to write (templated) functions that have the insurance that it will not trigger an overflow while only injecting the actual clamping logic into the function when it is actually necessary.
Could you give an actual code suggestion how you would implement that check in a manner that gives a clear error message to the developer?
There was a problem hiding this comment.
static_assert would do, no?
static_assert(!std::is_same<To, From>::value, "Should not try to clamp to the same type")or similar
There was a problem hiding this comment.
I'm not fully convinced yet it's a good idea to assert this. It would require removing some of the clamps, and for some of them keeping them might be better/safer; aa54690
newgrf_industries: the callback should return 16 bits; now it requires removing the clamp, but when we change the waiting value in the future things will break while we could have guarded against it in advance.
station_cmd: even nastier; the code will just work without the clamping, also in the future but it will cause bits to get messed up, yielding potentially weird station ratings. Keeping the clamp would prevent an issue if the time since pickup becomes larger.
vehicle_gui: adding and subtracting ages of vehicles... that's going to take a long while to get into problems. So removing the clamp here seems fine.
I could even imagine, especially in the API towards NewGRFs, that we conciously add clamping regardless of the type and let the compiler, based on the actual types at that moment to determine whether a clamp is needed or not. Always the same logic, so less chance of things going awry.
There was a problem hiding this comment.
How harmful is an unnecessary clamp? It can be difficult to know if the compiler has promoted an operation up to int, or a type-change later on somewhere else could have undesirable effects.
I feel an unnecessary clamp is less harmful than a missing clamp that was not originally put in place as it was unnecessary at the time.
There was a problem hiding this comment.
The ClampTo can be written in such a manner that if the "to" type can hold all values of "from", e.g. from uint16_t to int32_t to simply nothing. ClampTo would likely get optimized away with even low optimisation levels.
int to uint32_t would at most be optimized to a max(0, value), so one comparison, and then it's up to the compiler to optimize that away if it has more knowledge.
There was a problem hiding this comment.
Yes, I was thinking more in terms of a rationale for prohibiting clamping to the same type, rather than the resulting compiled code which more than likely would be optimized away.
"We shouldn't do it because it's not strictly necessary" is quite weak when it comes to C++ :-)
There was a problem hiding this comment.
I changed it back to just do nothing when no actual clamping is needed. That indeed feels like the best way forward by allowing adding Clamp pre-emptively to prevent issues at a later time. In a sense it would be documenting in actual code what the behaviour of the result is expected to be, regardless of the current input type.
rubidium42
force-pushed
the
clampto
branch
3 times, most recently
from
59f69af
to
cb41a55
Compare
|
The last commit seems to be fixing things that aren't otherwise touched by this PR, are they new errors/warnings? |
It's now the first commit, and to be honest most (but not all) of these are triggered when enabling precompiled headers. Technically the compiler complaining has a point, but for some reason it knew before it wasn't a problem or something. |
Motivation / Problem
We have
ClampToI32andClampToU16.We also have lots of variations of
min(<value>, <highest_value_of_type>)orClamp(<value>, <lowest_value_of_type>, <highest_value_of_type>). If you then want that function to return the requested type, the parameters become of that type as well, performing potential down casts.For example
Clamp<uint8_t>(257, 0, 255)will yield 2. SimilarlyClampToU16withMIN_INT64will yield 65535,or
ClampToI32yields -1 forMAX_UINT64.Description
Introduce a generic
ClampTo<integer_type>function that handles all the nitty gritty details. Likeuint64_ttoint64_twill not cast theuint64_tvalue just toint64_t, it will return the minimum of MAX_INT64 and the given value, alleviating the problem with casting where the very large numbers become negative.After that replace
ClampToI32andClampToU16with the new function, and introduce the function on more places where a similar idiom is used.Limitations
There could be more places that could benefit, that I might have overlooked.
Checklist for review
Some things are not automated, and forgotten often. This list is a reminder for the reviewers.