Fix some of the warnings from LGTM #9235
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rubidium42
force-pushed
the
lgtm
branch
3 times, most recently
from
515ca67
to
5729137
Compare
frosch123
reviewed
May 11, 2021
| @@ -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.
I think there can be NUM_CARGOS*NUM_STATIONS nodes. That would exceed 64k.
There was a problem hiding this comment.
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.
rubidium42
added
the
backport requested
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.
LordAro
approved these changes
May 15, 2021
TrueBrain
removed
the
backport requested
PeterN
added a commit
to PeterN/OpenTTD
that referenced
this pull request
Sep 9, 2023
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.
PeterN
added a commit
to PeterN/OpenTTD
that referenced
this pull request
Sep 9, 2023
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.
PeterN
added a commit
to PeterN/OpenTTD
that referenced
this pull request
Sep 9, 2023
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.
PeterN
added a commit
that referenced
this pull request
Sep 9, 2023
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.