google / benchmark

A microbenchmark support library
Apache License 2.0
8.94k stars 1.62k forks source link

Audit MSVC references in cmake files to consider clang++ #1669

Closed Maetveis closed 1 year ago

Maetveis commented 1 year ago

There are three major compilers on Windows targeting the MSVC ABI (i.e. linking with microsofts STL etc.):

The cmake variable MSVC is only set for the first two as it defined in terms of the CLI interface provided:

Set to true when the compiler is some version of Microsoft Visual C++ or another compiler simulating the Visual C++ cl command-line syntax.

(from cmake docs)

For many of the checks in the library its the ABI that matters not the cmdline, so check CMAKE_CXX_SIMULATE_ID too, if it is MSVC the current compiler is targeting the MSVC ABI. This handles clang++

Fixes: #1597

google-cla[bot] commented 1 year ago

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Maetveis commented 1 year ago

I don't think the sanitizer builds failing are relevant, this shouldn't change anything on Linux and all windows jobs pass.

dmah42 commented 1 year ago

I don't think the sanitizer builds failing are relevant, this shouldn't change anything on Linux and all windows jobs pass.

agreed. filed https://github.com/google/benchmark/issues/1670