Fix `-Wsuggest-override` warning when building benchmark with clang-cl on Windows

[SibiSiddharthan]

Summary

This patch adds '-Wno-suggest-override' flag to clang-cl.

Currently this flag gets added in the else() block for compilers
other than MSVC. Since CMake evaluates if(MSVC) true for clang-cl
this flag does not get added.

Let me know if it would be better if we move the
if(CXX_SUPPORTS_SUGGEST_OVERRIDE_FLAG) block outside if(MSVC).

[kbobyrev]

For benchmark modifications, we do (see README.md) we typically patch upstream and mention the change here while patching. Is this change something that has been committed to upstream or is this the modification that you came up with yourself?

[SibiSiddharthan]

In D101529#2726405, @kbobyrev wrote:

For benchmark modifications, we do (see README.md) we typically patch upstream and mention the change here while patching. Is this change something that has been committed to upstream or is this the modification that you came up with yourself?

This change has not been committed upstream and I came up with it myself (similar to lines 166-168). I would prefer if that snippet would be moved outside the if(MSVC) block.
Since this is my first commit to the project I wanted to project this issue and suggest a suitable fix for it.

Thank You

[kbobyrev]

Hey! If you're still interested in landing it, please make sure to land it into the upstream first and mention the revision here.

[SibiSiddharthan]

In D101529#2843405, @kbobyrev wrote:

Hey! If you're still interested in landing it, please make sure to land it into the upstream first and mention the revision here.

Hey, I have a question. When you say upstream do you mean here https://github.com/google/benchmark ?
If so, do I have to raise a pull request there and reference it here?. I do see that the benchmark under the llvm-project has changes that are not present in https://github.com/google/benchmark

[kbobyrev]

In D101529#2847820, @SibiSiddharthan wrote:

In D101529#2843405, @kbobyrev wrote:

Hey! If you're still interested in landing it, please make sure to land it into the upstream first and mention the revision here.

Hey, I have a question. When you say upstream do you mean here https://github.com/google/benchmark ?

Correct, upstream means the main benchmark repo.

If so, do I have to raise a pull request there and reference it here?.

The process looks as follows: you can open a PR in there, have it approved and landed in the main branch. Then you have the https://github.com/google/benchmark/commit/<YOUR_COMMIT_HASH> and you insert it into the Phabricator revision description, also mentioning it in the file where we have all the modifications.

I do see that the benchmark under the llvm-project has changes that are not present in https://github.com/google/benchmark

Yeah, this is unfortunately true but it isn't really a good practice :( If you open a PR and nobody looks into that and it doesn't get merged reasonably fast then it'd be OK to just land the change here, but I would be happy if you could open a PR. If there won't be any response from the maintainers in several days, we'll just land this here.

[lebedev.ri]

In D101529#2852271, @kbobyrev wrote:

In D101529#2847820, @SibiSiddharthan wrote:

In D101529#2843405, @kbobyrev wrote:

Hey! If you're still interested in landing it, please make sure to land it into the upstream first and mention the revision here.

Hey, I have a question. When you say upstream do you mean here https://github.com/google/benchmark ?

Correct, upstream means the main benchmark repo.

If so, do I have to raise a pull request there and reference it here?.

The process looks as follows: you can open a PR in there, have it approved and landed in the main branch. Then you have the https://github.com/google/benchmark/commit/<YOUR_COMMIT_HASH> and you insert it into the Phabricator revision description, also mentioning it in the file where we have all the modifications.

I do see that the benchmark under the llvm-project has changes that are not present in https://github.com/google/benchmark

Yeah, this is unfortunately true but it isn't really a good practice :( If you open a PR and nobody looks into that and it doesn't get merged reasonably fast then it'd be OK to just land the change here, but I would be happy if you could open a PR. If there won't be any response from the maintainers in several days, we'll just land this here.

FWIW i think all such PR's have been merged within hours of being opened.
Tides may change once said library starts depending on abseil though.

[SibiSiddharthan]

Hi there,

Looks like this issue has been fixed now by another commit. Closing this PR.

Thanks

llvm/utils/benchmark/CMakeLists.txt

if (BENCHMARK_ENABLE_LTO)
     string(REGEX REPLACE "[-/]INCREMENTAL" "/INCREMENTAL:NO" CMAKE_EXE_LINKER_FLAGS_RELWITHDEBINFO "${CMAKE_EXE_LINKER_FLAGS_RELWITHDEBINFO}")
     set(CMAKE_EXE_LINKER_FLAGS_RELWITHDEBINFO "${CMAKE_EXE_LINKER_FLAGS_RELWITHDEBINFO} /LTCG")
 
     set(CMAKE_CXX_FLAGS_MINSIZEREL "${CMAKE_CXX_FLAGS_MINSIZEREL} /GL")
     set(CMAKE_STATIC_LINKER_FLAGS_MINSIZEREL "${CMAKE_STATIC_LINKER_FLAGS_MINSIZEREL} /LTCG")
     set(CMAKE_SHARED_LINKER_FLAGS_MINSIZEREL "${CMAKE_SHARED_LINKER_FLAGS_MINSIZEREL} /LTCG")
     set(CMAKE_EXE_LINKER_FLAGS_MINSIZEREL "${CMAKE_EXE_LINKER_FLAGS_MINSIZEREL} /LTCG")
   endif()
+
+  # Turn off this warning for clang-cl builds
+  if(CLANG_CL)
+    if (CXX_SUPPORTS_SUGGEST_OVERRIDE_FLAG)
+      add_cxx_compiler_flag(-Wno-suggest-override)
+    endif()
+  endif()
 else()
   # Try and enable C++11. Don't use C++14 because it doesn't work in some
   # configurations.
   add_cxx_compiler_flag(-std=c++11)
   if (NOT HAVE_CXX_FLAG_STD_CXX11)
     add_cxx_compiler_flag(-std=c++0x)
   endif()