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
Fixes and code changes to reduce number of LGTM alerts #9296
Conversation
src/rail_cmd.cpp
Outdated
@@ -1281,10 +1281,10 @@ static bool CheckSignalAutoFill(TileIndex &tile, Trackdir &trackdir, int &signal | |||
* @param text unused | |||
* @return the cost of this operation or an error | |||
*/ | |||
static CommandCost CmdSignalTrackHelper(TileIndex tile, DoCommandFlag flags, uint32 p1, uint32 p2, const char *text) | |||
static CommandCost CmdSignalTrackHelper(TileIndex start_tile, DoCommandFlag flags, uint32 end_tile, uint32 p2, const char *text) |
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.
Why p1
-> end_tile
? Don't we always use the p1, p2
signature for all Cmd functions?
I would think it is better to rename the shadow p1
instead?
Similar to the start_tile
change btw.
Edit: some quick grepping in the source code, indeed, we always call it p1, p2
. We mostly call it tile
, with some exceptions. Maybe better to stick to that?
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.
Yes, p1 and p2 are often used for the Cmd functions. However, the parameter names are not part of the function declaration. Code wise it would, in my opinion, be better to have variable/parameter names that tell me something useful than that I would have to look it up. In this case it creates two extra variables just to store tile/p1 with a better name. By the way, looking at the function declaration now... it's not even a real command. Just a helper for a command so renaming p1
is technically less of an issue here I'd guess, and probably text
can even be removed.
In other cases there are constructs like TileArea(tile, p2)
. What is the meaning of p2 there? An encoded width and height? Or a tile? And when tile
is defined as the end tile of area-drag
why is it the first parameter? So tile
is the end... that's unexpected! In my opinion TileArea(start_tile, end_tile)
looks way more descriptive, and start_tile
is more descriptive than p2
.
I can as easily change it back and rename the p1 later down in this code to something else. Though with the other cases I would need to figure out another name for the tile
that is used to iterate over tiles in the TileArea. Could of course use t
, but that's not really descriptive on its own.
So, the main question is... what's the consensus? Keep tile
and p1
/p2
as required parameter names for commands? Or allow changing the parameter names to better suit the content.
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.
Keep as p1 & p2, IMO. doxygen comment is supposed to properly state what points to what, and ideally it would be done in the first few lines of the function anyway.
Ideally, It would just be two (or just one?) bitfield that magically separated the types/variables out as appropriate before it even got to the Cmd function - I think @nielsmh had some ideas about that?
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.
The current consensus clearly is p1, p2
, as a grep shows :) (140+ use it, only a handful don't). Personally I like that, as it makes it very easy to spot where the information comes from.
The idea behind it was that p1/p2 are always first cast to another variable, to assign it meaning. For example, in this case, you don't cast it to TileIndex
. That is not wrong, as it is an uint32
, just less verbose.
Either way, I think this is a conversation on a whole different level, and I do not think we should "sneak" that in a PR like this :) If we want to let go of p1, p2
we might want to do it in another PR and going through all 140-ish commands. That also makes it a lot easier for future-us to get what went on, instead of a commit that is seemly unrelated :)
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.
Changed it back, and renamed the other uses of tile
to current_tile
so it does not overlap with tile
from the parameters. I could not use t
in some places as there t
already referred to Town
.
…ially in a recursive function
Division by resize_y is already yielding an unsigned number, so when clicking in the WD_FRAMERECT_TOP you would already get a huge value, so sel would never be negative. So, leave sel an unsigned number and remove the <= check.
…o make Size() the same type to prevent infinite loops
…ructor that is not trivial This to prevent the default copy-assignment getting used when during the assignment also some other memory needs to be allocated as that would otherwise be freed.
Motivation / Problem
There are quite a few warnings coming from LGTM (https://lgtm.com/projects/g/OpenTTD/OpenTTD/?mode=tree) and it would be nice to resolve them. So in the future we might be able to add that to the commit checks.
Description
Depending on the issue removing some code, renaming variables, or marking default generated code as 'deleted'.
The commits usually try to solve most or all cases of a single type of alert.
Limitations
Not all warnings/recommendations are solved yet. But that's something for another PR.
Checklist for review
Some things are not automated, and forgotten often. This list is a reminder for the reviewers.