-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
Add -Wsuggest-destructor-override check #13213
base: main
Are you sure you want to change the base?
Conversation
d2c82f8
to
4abcd3e
Compare
I roughly remembered we have quite some places where destructor is not marked explicitly .... so maybe we need to mark them first to pass the new check? |
bd6ae86
to
d3729e9
Compare
d3729e9
to
e46727f
Compare
f29c2c2
to
f9cce94
Compare
f9cce94
to
51769c4
Compare
Yes we do need to update a bunch of destructors. "Quite some places" is an understatement |
a14b6b3
to
378d36a
Compare
Yeah sorry this turned out to be complicated, not just with gtest but with clang versions also. We primarily want to be sure our public API headers are clean of lots of warnings. We need to be prepared for however strictly our users choose to compile their own code. |
Summary
This was suggested in #13212 (comment). It should help reduce the discrepancy between our CI checks here and the CI checks that get run as part of releases to fbcode.
I am still not sure why the fbcode CI checked only complained about 2 instances of this missing
override
issue when there are clearly many, many more examples.Note that with
clang-10
, this-Wsuggest-destructor-override
option is not recognized, so I only add it forclang-13
in the MAKEFILE.Test Plan
These updated GitHub CI checks should pass, and we should no longer run into the same surprise at release time in the fbcode repo.