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: Remove last FOR_XXX macros #9368

Merged
merged 6 commits into from Jul 9, 2021
Merged

Codechange: Remove last FOR_XXX macros #9368

merged 6 commits into from Jul 9, 2021

Conversation

@glx22
Copy link
Contributor

@glx22 glx22 commented Jun 13, 2021

Motivation / Problem

I finally finished removing FOR_XXX macros.

(Now Intellisense is more usable)

Description

  • Replaced manually allocated HeightMap::h with a vector
  • Used a span for _sorted_cargo_specs sub-range of standard CargoSpec
  • Introduced an iterator for bitset (and used it)

Limitations

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 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')
src/cargotype.cpp Outdated Show resolved Hide resolved
Loading

private:
Tbitset bitset;
};
Copy link
Member

@TrueBrain TrueBrain Jun 14, 2021

Choose a reason for hiding this comment

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

Sadly, I have not enough knowledge to rate this part of the PR in any way :D @LordAro , mind giving this a look? Tnx!

Loading

Copy link
Member

@LordAro LordAro Jun 14, 2021

Choose a reason for hiding this comment

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

Dunno much either. Part of me wonders whether std::bitset might be useable somehow (though may well require a fair bit more refactoring to do so)

Loading

Copy link
Contributor

@rubidium42 rubidium42 Jun 19, 2021

Choose a reason for hiding this comment

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

Though bitset doesn't have iterators (or rather begin/end), so can it iterate over them? With my naive check you can't, and I think that was the whole point of making this iterator.

Loading

src/core/bitmath_func.hpp Show resolved Hide resolved
Loading

private:
Tbitset bitset;
};
Copy link
Contributor

@rubidium42 rubidium42 Jun 19, 2021

Choose a reason for hiding this comment

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

Though bitset doesn't have iterators (or rather begin/end), so can it iterate over them? With my naive check you can't, and I think that was the whole point of making this iterator.

Loading

* @tparam Tbitset Type of the bitset value.
*/
template <typename Tbitpos = uint, typename Tbitset = uint>
struct BitIterator {
Copy link
Contributor

@rubidium42 rubidium42 Jun 19, 2021

Choose a reason for hiding this comment

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

The name of the class does not imply that it only considers set bits, nor that it actually returns the position and not be bit value.

Loading

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Jun 27, 2021

I have no more comments. @rubidium42 : this okay with you too?

PS: really cool finally these really old macros are goneeeeeeee. Nice job @glx22 :D

Loading

Copy link
Member

@TrueBrain TrueBrain left a comment

I am going to assume the silence is a yes :)

Loading

@glx22 glx22 merged commit ce813ce into OpenTTD:master Jul 9, 2021
15 checks passed
Loading
@glx22 glx22 deleted the for_all branch Jul 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants