New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix some of the warnings from LGTM #9235
Conversation
515ca67
to
5729137
Compare
@@ -495,7 +495,7 @@ class LinkGraph : public LinkGraphPool::PoolItem<&_link_graph_pool> { | |||
* Get the current size of the component. | |||
* @return Size. | |||
*/ | |||
inline uint Size() const { return (uint)this->nodes.size(); } | |||
inline uint16 Size() const { return (uint16)this->nodes.size(); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there can be NUM_CARGOS*NUM_STATIONS nodes. That would exceed 64k.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that were to be the case, then the code itself is already broken by means of an infinite loop. Almost everywhere where this size is used, it is compared in a loop to uint16/NodeID (even in most places size is read into a uint size local variable).
So let Size() be 65536, then none of those loops would ever terminate as 65535+1=0 which is < 65536. Thus practically this change would make it slightly less broken, as in the linkgraph gets fracked instead of the game hangs.
If it really needs to be > 64k, then that'd require a savegame bump and quite a bit of effort.
Practically the length of the handlers not being equal to the number of features is the problem as it means something was forgotten when adding a new feature, so static assert to that and let the existing check on the feature number take care of invalid data from the NewGRFs.
TextEffectID was promoted to size_t in OpenTTD#9235, when it was used in loops. However since then the relevant code uses range-for or iterators instead.
TextEffectID was promoted to size_t in OpenTTD#9235, when it was used in loops. However since then the relevant code uses range-for or iterators instead.
TextEffectID was promoted to size_t in OpenTTD#9235, when it was used in loops. However since then the relevant code uses range-for or iterators instead.
TextEffectID was promoted to size_t in #9235, when it was used in loops. However since then the relevant code uses range-for or iterators instead.
Motivation / Problem
Not having warnings from static analysers such as LGTM is something to aim for, especially when they point to real bugs or prevent them from ever happening.
Description
Trying to fix the warnings by using safer routines or fixing the code where needed.
For the backport: only "Fix: [Network] Check on CIDR for netmask check considered everything valid" needs a backport as that is an actual bug, the others are more theoretical issues.
Limitations
This solves only a small set of warnings, however this is done to keep the PR size down.
Checklist for review
Some things are not automated, and forgotten often. This list is a reminder for the reviewers.