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

Centralize compiler detection for GCC, clang, MSVC and MINGW32. #2410

Draft
wants to merge 9 commits into
base: devel
Choose a base branch
from

Conversation

HoseynHeydari
Copy link

Description

Macros detecting compilers GCC, clang, MSVC and MINGW32 added to catch_compiler_capabilities.hpp .
New macros are replaced with former conditions that detect the compiler.
If detecting compilers needs more conditions we can just change macros
at catch_compiler_capabilities.hpp instead of multi places in code.

GitHub Issues

Closes #2094

@codecov
Copy link

codecov bot commented Apr 15, 2022

Codecov Report

Merging #2410 (e1827f7) into devel (7b93a20) will not change coverage.
The diff coverage is n/a.

❗ Current head e1827f7 differs from pull request most recent head 56d04d2. Consider uploading reports for the commit 56d04d2 to get more accurate results

@@           Coverage Diff           @@
##            devel    #2410   +/-   ##
=======================================
  Coverage   91.10%   91.10%           
=======================================
  Files         157      157           
  Lines        7414     7414           
=======================================
  Hits         6754     6754           
  Misses        660      660           

@HoseynHeydari
Copy link
Author

HoseynHeydari commented Apr 15, 2022

Some includes should be removed before merging. :|
I try to handle it by review but there were a lot of files.

@HoseynHeydari HoseynHeydari marked this pull request as draft April 17, 2022 21:20
@HoseynHeydari
Copy link
Author

Some lines missing while renaming macro from CATCH_COMPILER_MSC to CATCH_COMPILER_MSVC.
After testing with clang and MSVC compiler on my machine I will push new changes.

#ifndef CATCH_COMPILER_DETECTIONS_HPP_INCLUDED
#define CATCH_COMPILER_DETECTIONS_HPP_INCLUDED

#if defined(__GNUC__) && !defined(__clang__) && !defined(__ICC) && !defined(__CUDACC__) && !defined(__LCC__)
Copy link
Member

Choose a reason for hiding this comment

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

This approach isn't going to scale well, as pretty much every new or obscure compiler masquerades for GCC, so this line is going to keep getting longer and longer.

Instead, let's have a macro CATCH_COMPILER_DETECTED, that get's set when a compiler was picked. This would simplify GCC (and also clang) to something like

#if defined(__GNUC__) and !defined(CATCH_COMPILER_DETECTED)
#define CATCH_COMPILER_GCC
#endif

#define CATCH_COMPILER_GCC
#endif

#if defined(__clang__)
Copy link
Member

Choose a reason for hiding this comment

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

This definitely doesn't filter out all the different compilers that like to pretend they are Clang, e.g. IBM XL. See above for a good way to fix this.

@horenmar
Copy link
Member

I think this PR is currently trying to do too much. I'd recommend starting by merging the reworked compiler detection macros and the code can be converted to use them iteratively later on.

@@ -223,12 +225,12 @@ TEST_CASE( "Comparisons with int literals don't warn when mixing signed/ unsigne
// Disable warnings about sign conversions for the next two tests
// (as we are deliberately invoking them)
// - Currently only disabled for GCC/ LLVM. Should add VC++ too
#ifdef __GNUC__
Copy link
Author

Choose a reason for hiding this comment

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

This line means if compiler is GNUC or clang.
But I just replaced it by CATCH_COMPILER_GCC cuases compiler warning -> error.
I'm searching for this situations in code.

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.

Refactor and centralize compiler detection
2 participants