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

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

Merged
merged 2 commits into from
Aug 26, 2024
Merged

Conversation

HexDecimal
Copy link
Collaborator

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
Copy link
Collaborator Author

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.

@HexDecimal HexDecimal marked this pull request as ready for review August 21, 2024 09:28
@rmtew
Copy link
Owner

rmtew commented Aug 21, 2024

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

@HexDecimal
Copy link
Collaborator Author

HexDecimal commented Aug 21, 2024

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
Copy link
Owner

rmtew commented Aug 22, 2024

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
Copy link
Collaborator Author

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.

The simplest solution was to add `using` to replace the macros exactly
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.

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

Refactor Grammar.acc to use std:: namespace
@HexDecimal HexDecimal marked this pull request as draft August 22, 2024 23:04
Not even a using alias in the headers anymore.

Found some subtle edge cases by doing this.
Also updates min/max in unused code.

Use `define NOMINMAX` instead of `undef` to prevent Windows headers of defining min/max.
@HexDecimal HexDecimal marked this pull request as ready for review August 23, 2024 00:45
@HexDecimal
Copy link
Collaborator Author

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
Copy link
Owner

rmtew commented Aug 26, 2024

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
Copy link
Collaborator Author

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
Copy link
Owner

rmtew commented Aug 26, 2024

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
Copy link
Collaborator Author

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
Copy link
Owner

rmtew commented Aug 26, 2024

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

@rmtew rmtew merged commit eaa72bf into rmtew:master Aug 26, 2024
6 checks passed
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.

Reserved keyword redefinitions
2 participants