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

Wrap all std::min and std::max calls in parentheses #2159

Merged
merged 1 commit into from
Feb 20, 2021

Conversation

ScottHutchinson
Copy link
Contributor

Fixes x86 MSVC v19.28 compiler error C2589 on Windows.

Many of the std::min and std::max calls were already wrapped in parentheses.

Some of the errors were for benchmark builds only.

@codecov
Copy link

codecov bot commented Jan 23, 2021

Codecov Report

Merging #2159 (6ba264b) into v2.x (68975e3) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             v2.x    #2159   +/-   ##
=======================================
  Coverage   88.79%   88.79%           
=======================================
  Files         138      138           
  Lines        5683     5683           
=======================================
  Hits         5046     5046           
  Misses        637      637           

@horenmar
Copy link
Member

Ok, thanks.

@horenmar horenmar merged commit b025a00 into catchorg:v2.x Feb 20, 2021
@ferdnyc
Copy link
Contributor

ferdnyc commented Sep 10, 2021

@horenmar @ScottHutchinson
Just FYI, a possibly-more-robust solution to this — so that everyone doesn't have to live in continual fear that someone will add an unparenthetized std::min() or std::max()call to the code at some point in the future and re-break things — is to #define NOMINMAX wherever this issue comes up. (Or just everywhere, when building on Windows / with MSVC.)

That will deactivate the min() and max() preprocessor macros stupidly defined in WinDefs.h (which is presumably being included directly or indirectly); it seems those bad macros are the root cause of these issues. (More on the issue.)

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.

3 participants