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

Convert min/max macros to std::min/max #23

Merged
merged 1 commit into from
Apr 17, 2024

Conversation

th1000s
Copy link
Contributor

@th1000s th1000s commented Apr 16, 2024

Added explicit <float> or <int> template argument where needed.
Add -DNOMINMAX define for Windows.


Commit message was:

These only cause trouble in stdlib headers, such as:

/usr/include/c++/12/limits:1828:13: error: expected unqualified-id before ‘noexcept’
 1828 |       max() _GLIBCXX_USE_NOEXCEPT { return __LDBL_MAX__; }
      |             ^~~~~~~~~~~~~~~~~~~~~
Descent3/d3music/musicapi.cpp: In function ‘void D3MusicSetVolume(float)’:
Descent3/d3music/musicapi.cpp:167:57: error: ‘std::numeric_limits<float>::min’ cannot be used as a function
  167 |   const float kEpsilon = std::numeric_limits<float>::min();
      |                          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~

plus there is the whole NOMINMAX situation on Windows.

And don't introduce <algorithm> just for that.

Automated via:

$ sed -i 's/_\?_\?min *(\(.*\),/D3_min(\1,/' `git grep -l min`
$ sed -i 's/_\?_\?max *(\(.*\),/D3_max(\1,/' `git grep -l max`

plus remove duplicates from lib/Macros.h + lib/linux/linux_fix.h and fix netgames/dmfc/dmfcstats.cpp (2x min on one line) manually.


Or, any other name, maybe just min_ / max_?

@kevinbentley
Copy link
Member

kevinbentley commented Apr 16, 2024

Is there a reason not to use std::min? numeric_limits::min is the minimum value a float can store, whereas std::min(a,b) is the lesser of the two numbers.

@winterheart
Copy link
Collaborator

winterheart commented Apr 16, 2024

Agree, would be better use std::min / std::max and get rid min / max macros altogether. In code there also _min, __min, _max, __max variations.

@th1000s
Copy link
Contributor Author

th1000s commented Apr 16, 2024

numeric_limits::min()

That might just be the pre-processor replacing stuff where it should not.

So far I only saw <string> from the stdlib, so I thought I'd avoid <algorithm> which famously increases compile times.

However, moving to std::max() triggers a lot of no matching function for call to ‘max(int, float)’ (or float, ushort, int, ubyte in various combinations). For the time being renaming the macro is the quicker way to get Linux to compile.

@winterheart
Copy link
Collaborator

Well, there need a casting into desired type. Since there anyway a lot of renaming, why don't go little further and use standardized functions?

@th1000s
Copy link
Contributor Author

th1000s commented Apr 17, 2024

Renaming the macro was mostly done to get the code to compile, small steps :)

Fully switching to std::max() is less automated and requires different casts -- max(int, float) should not become max(static_cast<double>(a), static_cast<double>(b)). And even with the correct types and just float(a) C-style casts, that makes the call sites a bit unwieldy.

Instead, maybe replace the macro with a custom d3_max() template using std::common_type?

@kevinbentley
Copy link
Member

You can specialize std::min like this:

int a = 123;
float b = 100.01;
float x = std::min<float>(a,b);

IMO, that's the best way to do it.

@th1000s th1000s changed the title Rename min/max macros to D3_min/D3_max Convert min/max macros to std::min/max Apr 17, 2024
@th1000s
Copy link
Contributor Author

th1000s commented Apr 17, 2024

It is now templated where needed. <float> was only added (in a somewhat automated way) where at least one component was a float / double according to gcc, I also converted a few 1.0 to 1.0f or added <int> manually.

@Lgt2x
Copy link
Member

Lgt2x commented Apr 17, 2024

@th1000s can you please rebase on master branch ?

@th1000s th1000s force-pushed the minmax branch 4 times, most recently from 8ba8042 to f73b156 Compare April 17, 2024 19:40
Added explicit <float> or <int> template argument where needed.
Add -DNOMINMAX define for Windows.
@Lgt2x Lgt2x merged commit 494d588 into DescentDevelopers:main Apr 17, 2024
4 checks passed
@Lgt2x Lgt2x mentioned this pull request Apr 17, 2024
JeodC pushed a commit that referenced this pull request Apr 28, 2024
Convert min/max macros to std::min/max
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.

4 participants