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

Specify __cdecl on main/wmain, as required by x86 (fixes #2486). #2487

Merged
merged 5 commits into from
Aug 6, 2022

Conversation

davidmatson
Copy link

No description provided.

@codecov
Copy link

codecov bot commented Jul 25, 2022

Codecov Report

Merging #2487 (b12c7a8) into v2.x (20d413b) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             v2.x    #2487   +/-   ##
=======================================
  Coverage   90.08%   90.08%           
=======================================
  Files         113      113           
  Lines        5040     5040           
=======================================
  Hits         4540     4540           
  Misses        500      500           

@davidmatson davidmatson changed the title Specify __cdecl on wmain, as required by x86 (fixes #2486). Specify __cdecl on main/wmain, as required by x86 (fixes #2486). Jul 26, 2022
@horenmar
Copy link
Member

horenmar commented Aug 5, 2022

I was gonna say that I am surprised this passes CI, but v2 used to use travis for non-Windows CI and that is dead.

Sprinkling __cdecl around will not compile with non-MSVC compilers, so it will need to be conditional. Alternatively, the warning that causes the compilation failure with /WX can be supressed.

@davidmatson
Copy link
Author

Sprinkling __cdecl around will not compile with non-MSVC compilers, so it will need to be conditional.

Done - thanks for taking a look!

@horenmar horenmar added the Building and Packaging Issues affecting build/packaging scripts and utilities label Aug 6, 2022
@horenmar horenmar merged commit 14bc25b into catchorg:v2.x Aug 6, 2022
@davidmatson davidmatson deleted the fixX86WMainCDecl branch August 6, 2022 20:02
@@ -13,12 +13,20 @@

#ifndef __OBJC__

#ifndef CATCH_INTERNAL_CDECL
#ifdef _MSC_VER
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO it would be a good idea to additionally constrain this even more to just 32bit x86 with the predefined macro _M_IX86.

_M_IX86 Defined as the integer literal value 600 for compilations that target x86 processors. This macro isn't defined for x64 or ARM compilation targets.

Copy link
Author

Choose a reason for hiding this comment

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

I believe the convention, when specifying __cdecl, is always to specify it though rather than making it arch-specific. (I believe the requirement is for all architectures, it's just technically no different on some so it doesn't cause a problem in practice if it's not there.)

For example:
"On ARM and x64 processors, __cdecl is accepted but typically ignored by the compiler."
https://docs.microsoft.com/en-us/cpp/cpp/cdecl?view=msvc-170

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Building and Packaging Issues affecting build/packaging scripts and utilities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants