[PATCH] D84244: [clang] Disable -Wsuggest-override for unittests/

[Logan Smith via Phabricator via cfe-commits]

logan-5 added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84244/new/

https://reviews.llvm.org/D84244



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84244: [clang] Disable -Wsuggest-override for unittests/

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

> 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84244/new/

https://reviews.llvm.org/D84244



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84244: [clang] Disable -Wsuggest-override for unittests/

[Logan Smith via Phabricator via cfe-commits]

logan-5 added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84244/new/

https://reviews.llvm.org/D84244



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84244: [clang] Disable -Wsuggest-override for unittests/

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84244/new/

https://reviews.llvm.org/D84244



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84244: [clang] Disable -Wsuggest-override for unittests/

[Logan Smith via Phabricator via cfe-commits]

logan-5 added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84244/new/

https://reviews.llvm.org/D84244



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84244: [clang] Disable -Wsuggest-override for unittests/

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84244/new/

https://reviews.llvm.org/D84244



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84244: [clang] Disable -Wsuggest-override for unittests/

[Logan Smith via Phabricator via cfe-commits]

logan-5 added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84244/new/

https://reviews.llvm.org/D84244



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84244: [clang] Disable -Wsuggest-override for unittests/

[Hans Wennborg via Phabricator via cfe-commits]

hans added inline comments.



Comment at: clang/unittests/CMakeLists.txt:14
+if (CXX_SUPPORTS_SUGGEST_OVERRIDE_FLAG)
+  add_definitions("-Wno-suggest-override")
+endif()

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84244/new/

https://reviews.llvm.org/D84244



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84244: [clang] Disable -Wsuggest-override for unittests/

[Logan Smith via Phabricator via cfe-commits]

logan-5 added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84244/new/

https://reviews.llvm.org/D84244



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84244: [clang] Disable -Wsuggest-override for unittests/

[David Blaikie via Phabricator via cfe-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

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)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84244/new/

https://reviews.llvm.org/D84244



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84244: [clang] Disable -Wsuggest-override for unittests/

[Logan Smith via Phabricator via cfe-commits]

logan-5 added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84244/new/

https://reviews.llvm.org/D84244



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84244: [clang] Disable -Wsuggest-override for unittests/

[Logan Smith via Phabricator via cfe-commits]

logan-5 added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84244/new/

https://reviews.llvm.org/D84244



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84244: [clang] Disable -Wsuggest-override for unittests/

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added a comment.

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?)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84244/new/

https://reviews.llvm.org/D84244



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84244: [clang] Disable -Wsuggest-override for unittests/

[Logan Smith via Phabricator via cfe-commits]

logan-5 created this revision.
logan-5 added a reviewer: mgorny.
logan-5 added a project: clang.
Herald added a subscriber: cfe-commits.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D84244

Files:
  clang/unittests/CMakeLists.txt


Index: clang/unittests/CMakeLists.txt
===
--- clang/unittests/CMakeLists.txt
+++ clang/unittests/CMakeLists.txt
@@ -10,6 +10,10 @@
   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


Index: clang/unittests/CMakeLists.txt
===
--- clang/unittests/CMakeLists.txt
+++ clang/unittests/CMakeLists.txt
@@ -10,6 +10,10 @@
   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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits