rmtew / incursion-roguelike

The legendary computer game Incursion: Hall of the Goblin King!
https://incursion-roguelike.net/
Other
9 stars 1 forks source link

Replace min/max macros with standard C++ #22

Closed HexDecimal closed 3 months ago

HexDecimal commented 3 months ago

The simplest solution was to add a using alias to replace the macros with the C++ functions.

Platform toolset has been increased to v143. This was probably not needed for this PR but it is needed for C++17 and libtcod.

An explicit type must be given when int constants are mixed with smaller types. This mostly meant going around and adding <int> to everything.

The sources which needed to be updated includes Grammar.acc.

Unless there's anything unexpected, this closes #12

I'll look at this again when I'm more awake before marking this as ready.

HexDecimal commented 3 months ago

Wanted to be sure that Grammar.acc supported the std:: namespace. In case I want to later remove the using alias and have all sources use std::min and std::max explicitly.

I can spend more time on this if you want, or you can merge this now. No doubt there's several conversion warnings from numbers being cast to int then downcast back to int16 or smaller.

rmtew commented 3 months ago

To be clear, modaccent works fine and regenerates yygram.cpp with the modified grammar.acc file?

HexDecimal commented 3 months ago

On the latest commit I edited grammar.acc and regenerated yygram.cpp from the VS Win32 build process to be sure that whatever it uses can handle the :: syntax. I skipped adding the line directive edits to Git, would be nice to make those platform independent.

Then tested loading a game on Incursion x64, did NOT test Incursion -compile.

rmtew commented 3 months ago

Sorry, I forgot to comment. Before this can be merged, this must be tested with compile and a short game played verifying the module is valid. The grammar is used to build the compiler.

HexDecimal commented 3 months ago

Was able to play a short game on this PR without any noticeable issues other than losing my saves from the version bump on master.

HexDecimal commented 3 months ago

I was able to do what I wanted and switch everything over to using the standard namespace. This is rebased onto the commits which verify generated files, and I've been play-testing on this branch.

When are you comfortable merging this?

rmtew commented 3 months ago

I want to do a branch shuffle. On one hand we want to be merging things like this and working towards an alpha release with them in, and on the other there should always be a stable codebase to do releases from with critical bug fixes.

So I kind of want to:

  1. Branch master to dev, perhaps after merging this.
  2. Force push master back before cleanup for a libtcod change-over. Cherry-pick in essential stable changes.
  3. Build libtcod 1.6.0 and verify that it works as well as it did for 0.6.9Y19.
  4. Do a 0.6.9Y20 release with recent bug fixes (2022 compiler, 2048 stack variable).

Then we have a viable project that can do releases with a known stability now. However, the goal is not to compete with the dev branch, that is a waste of time and effort. master would only have crash bugs merged and should be able to release often with minimal effort and minimal testing, any feature changes in addition to code modernisation would happen in dev.

HexDecimal commented 3 months ago

Instead of force pushing onto master again you should instead make a stable or releases branch forked from the last tag you consider "stable". Cherry-picking or backporting critical bug-fixes can work with tags alone if that's the goal.

rmtew commented 3 months ago

Yes, good idea, let's go with stable. That's less work. What do you mean by "Cherry-picking or backporting critical bug-fixes can work with tags alone if that's the goal." ?

HexDecimal commented 3 months ago

It's perhaps complex compared to a stable branch, but you can fork off a tag, add the fixes, then tag the result as a new release, but that is more for semantic versioning than for regular serial releases.

rmtew commented 3 months ago

Well, we have a plan for the branches. I'll merge this.