[clang] Disable -Wsuggest-override for unittests/

[logan-5]

Summary

Yesterday I enabled -Wsuggest-override in the main LLVM build and have been fighting with the -Werror bots ever since. The key culprits making this difficult are googletest and googlemock, which do not use the override keyword in their sources, so any files that include them are met with massive warning (or error, in the case of -Werror) spam.

I've been going through and playing whack-a-mole by adding -Wno-suggest-override to directories that have code that uses gtest and/or gmock; this approach is feeling increasingly inelegant the more I do it, but all the patches I've submitted for review have been LGTM'd so far.

I'm wondering if I should do this a different way, or if it's fine to just proceed along this path until the bots are green again.

Thank you for your review.

[• Quuxplusone]

For GTest and GMock specifically, it would be a good medium-term idea to try to upstream patches into those libraries. I did eventually have success upstreaming fixes for -Wdeprecated into GTest, for example: https://github.com/google/googletest/pull/2815
Disabling the warning in unittests for now still seems like a good short-term fix though.

(Although I'm a bit confused; I thought -Wsuggest-override was off-by-default? Is it part of -Wall, and do unittests add -Wall to their command line or something?)

[logan-5]

In D84244#2164537, @Quuxplusone wrote:

For GTest and GMock specifically, it would be a good medium-term idea to try to upstream patches into those libraries. I did eventually have success upstreaming fixes for -Wdeprecated into GTest, for example: https://github.com/google/googletest/pull/2815

Nice, that's encouraging. That does sound like a good solution for slightly further down the road.

(Although I'm a bit confused; I thought -Wsuggest-override was off-by-default? Is it part of -Wall, and do unittests add -Wall to their command line or something?)

After I implemented the warning and it landed, people wanted it explicitly turned on in the LLVM build, so then I did that. This is part of the fallout from that.

[logan-5]

Committing this now to fix the bots, as per discussion in https://reviews.llvm.org/D84126. I'll happily revert and approach it differently later if anyone requests that.

[dblaikie]

Looks OK (for future reference, this sort of stuff should've been cleaned up before enabling the flag so as to avoid this kind of hurry/breakage/etc)

[logan-5]

In D84244#2165592, @dblaikie wrote:

Looks OK (for future reference, this sort of stuff should've been cleaned up before enabling the flag so as to avoid this kind of hurry/breakage/etc)

Loud and clear. I honestly thought I had sifted through and dealt with all these before enabling it, but I was wrong. My mistake, I'm sorry about that.

[hans]

clang/unittests/CMakeLists.txt

14

Because it's added as a define, this gets passed to rc.exe in Windows builds, which is used for some clang-tools-extra unittest target, which then proceeds to fail with:

fatal error RC1106: invalid option: -o-suggest-override

(See e.g. https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8874065661026292624/+/steps/package_clang/0/stdout)

I don't know why this doesn't happen to other flags, but I think there's been enough fallout from this now that it should be reverted while that's figured out. Reverting in 3eec65782575a1284391e447142fd004dd5de4a9.

[logan-5]

Thanks for reverting--I agree that that's the right move at this point.

Pretty much totally out my depth here, and I don't have a way of debugging the Windows issue, so I'm not sure how to proceed. I'm tempted to just let this whole thing rest for now and maybe try again sometime in the future.

I didn't realize quite how much I was biting off by volunteering to do this. I want to apologize again for all the trouble.

[hans]

In D84244#2167602, @logan-5 wrote:

Thanks for reverting--I agree that that's the right move at this point.

Pretty much totally out my depth here, and I don't have a way of debugging the Windows issue, so I'm not sure how to proceed. I'm tempted to just let this whole thing rest for now and maybe try again sometime in the future.

I don't really know why this doesn't happen with other warning flags, but I think it would be better to add flags like this with add_compile_options rather than add_compile_definitions. I imagine that would prevent it from reaching rc.exe.

I didn't realize quite how much I was biting off by volunteering to do this. I want to apologize again for all the trouble.

No worries, these things happen. I'd say a lot of the trouble was caused by our build system, and also that lack of a good system to try out patches before landing.

[logan-5]

In D84244#2167625, @hans wrote:

I don't really know why this doesn't happen with other warning flags, but I think it would be better to add flags like this with add_compile_options rather than add_compile_definitions. I imagine that would prevent it from reaching rc.exe.

That sounds pretty reasonable. Without any way of testing the Windows setup myself though, I don't feel comfortable making that change.

[dblaikie]

In D84244#2167661, @logan-5 wrote:

In D84244#2167625, @hans wrote:

I don't really know why this doesn't happen with other warning flags, but I think it would be better to add flags like this with add_compile_options rather than add_compile_definitions. I imagine that would prevent it from reaching rc.exe.

That sounds pretty reasonable. Without any way of testing the Windows setup myself though, I don't feel comfortable making that change.

Sometimes it's OK to commit stuff an see how it goes on the buildbots - doing so out of peak times and with the intention of rolling back quickly/immediately if the buildbots report breakage is sometimes the best tool we have (not great, but such is the way of things).

Alternatively @hans might be able to test it for you before that, or have other ideas/suggestions.

[logan-5]

In D84244#2167663, @dblaikie wrote:

Sometimes it's OK to commit stuff an see how it goes on the buildbots - doing so out of peak times and with the intention of rolling back quickly/immediately if the buildbots report breakage is sometimes the best tool we have (not great, but such is the way of things).

In that case, I've enabled it again using add_compile_options instead of add_definitions. I've got my finger hovering over the button to revert.

[hans]

In that case, I've enabled it again using add_compile_options instead of add_definitions. I've got my finger hovering over the button to revert.

(For reference, the re-commit is 77e0e9e.)

Looks good to me so far. We haven't tried doing any full builds yet, but I checked for the specific problem I hit yesterday (building ClangApplyReplacementsTests) and that's working fine now.

[logan-5]

In D84244#2169142, @hans wrote:

Looks good to me so far. We haven't tried doing any full builds yet, but I checked for the specific problem I hit yesterday (building ClangApplyReplacementsTests) and that's working fine now.

Glad to hear that. I'll be keeping my eye on things today, but they look good so far to me too.

clang/unittests/CMakeLists.txt

 add_custom_target(ClangUnitTests)
 set_target_properties(ClangUnitTests PROPERTIES FOLDER "Clang tests")
 
 if(CLANG_BUILT_STANDALONE)
   # LLVMTestingSupport library is needed for some of the unittests.
   if (EXISTS ${LLVM_MAIN_SRC_DIR}/lib/Testing/Support
       AND NOT TARGET LLVMTestingSupport)
     add_subdirectory(${LLVM_MAIN_SRC_DIR}/lib/Testing/Support
       lib/Testing/Support)
   endif()
 endif()
 
+if (CXX_SUPPORTS_SUGGEST_OVERRIDE_FLAG)
+  add_definitions("-Wno-suggest-override")
+endif()
+
 # add_clang_unittest(test_dirname file1.cpp file2.cpp)
 #
 # Will compile the list of files together and link against the clang
 # Produces a binary named 'basename(test_dirname)'.
 function(add_clang_unittest test_dirname)
   add_unittest(ClangUnitTests ${test_dirname} ${ARGN})
 endfunction()