Skip to content
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

Codechange: use standard int types #10733

Merged
merged 2 commits into from Jul 19, 2023
Merged

Conversation

rubidium42
Copy link
Contributor

@rubidium42 rubidium42 commented Apr 28, 2023

Motivation / Problem

Using custom typedef for types that are standard types.
Mostly (u)intXX that can become (u)intXX_t, but also WChar becoming char32_t.

Description

Automatic replacements; script in commit message.
Manual removal of the typedefs from stdafx.h.

Limitations

Breaks quite some patches.

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR touches english.txt or translations? Check the guidelines
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@rubidium42 rubidium42 changed the title Codechange: use standard uint types Codechange: use standard int types Apr 28, 2023
static const uint8 ANIM_STATUS_NON_LOOPING = 0x00; ///< Animation is not looping.
static const uint8 ANIM_STATUS_LOOPING = 0x01; ///< Animation is looping.
static const uint8 ANIM_STATUS_NO_ANIMATION = 0xFF; ///< There is no animation.
static const uint8_t ANIM_STATUS_NON_LOOPING = 0x00; ///< Animation is not looping.

Check notice

Code scanning / CodeQL

Unused static variable Note

Static variable ANIM_STATUS_NON_LOOPING is never read.
@rubidium42 rubidium42 force-pushed the big-diff branch 2 times, most recently from 10bdd7a to 9dd1138 Compare April 28, 2023 21:56
@TrueBrain
Copy link
Member

I think this would help for new developers to join in, as this makes this project more like other C++ projects. Similar to the efforts of converting to std::string etc; it just makes things easier for new joiners.

But, it is a total patch-killer, and as such, I think it is good to involve patchpacks; either by supplying seds or something, so they can go through a similar conversion.

@rubidium42
Copy link
Contributor Author

I think this would help for new developers to join in, as this makes this project more like other C++ projects. Similar to the efforts of converting to std::string etc; it just makes things easier for new joiners.

But, it is a total patch-killer, and as such, I think it is good to involve patchpacks; either by supplying seds or something, so they can go through a similar conversion.

The probably should wait to just before branching 14, or at least just before the first beta. Regarding the sed, that's in the second paragraph of the automatic change commit of this PR.
And yes, the commits need to get a better description before this gets merged. But alas, it's more proof-of-work and something to get the discussion going than it already being polished to get to be merged.

@TrueBrain
Copy link
Member

The probably should wait to just before branching 14, or at least just before the first beta. Regarding the sed, that's in the second paragraph of the automatic change commit of this PR. And yes, the commits need to get a better description before this gets merged. But alas, it's more proof-of-work and something to get the discussion going than it already being polished to get to be merged.

Well, what I more meant is: we should actual talk to patchpack owners; that kind of "involving". If for them this means it is a HUGE hassle to keep up-to-date with upstream, we shouldn't do this.

@PeterN
Copy link
Member

PeterN commented Apr 29, 2023

I would like to see this if possible -- I had done global search & replaces to see what it looks like :-)

However we can at least start to use the correct types piecemeal for future changes?

Copy link
Member

@TrueBrain TrueBrain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everyone I talked to signed off on this, as in:

@JGRennison indicated it doesn't bother him, and he will deal with it.
@PeterN, myself and OP would like to see this change.
Nobody else spoke up against it.

In general I think this is a good improvement, as everyone these days assume uint32_t. And as these days it works on all OSes, why not convert the rest to it so we can no longer use uint32 at all. This also makes things just slightly easier for new contributors, as it complies with modern expectations of a codebase.

And anyone else that runs a patchpack, there is enough information in this PR to do this on your own code too, saving you a lot of trouble. But can't deny, this will hurt a bit.

But so yeah, let's do this!

(you most likely have to check if this needs a rebase; just wanted to give the approval to make a point :D )

…32_t

for i in `find src -type f|grep -v 3rdparty/fmt|grep -v 3rdparty/catch2|grep -v 3rdparty/opengl|grep -v stdafx.h`; do sed 's/uint16& /uint16 \&/g;s/int8\([ >*),;[]\)/int8_t\1/g;s/int16\([ >*),;[]\)/int16_t\1/g;s/int32\([ >*),;[]\)/int32_t\1/g;s/int64\([ >*),;[]\)/int64_t\1/g;s/ uint32(/ uint32_t(/g;s/_uint8_t/_uint8/;s/Uint8_t/Uint8/;s/ft_int64_t/ft_int64/g;s/uint64$/uint64_t/;s/WChar/char32_t/g;s/char32_t char32_t/char32_t WChar/' -i $i; done
@rubidium42 rubidium42 merged commit 461b4b8 into OpenTTD:master Jul 19, 2023
20 checks passed
@rubidium42 rubidium42 deleted the big-diff branch July 19, 2023 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants