Skip to content
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

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

archang19
Copy link
Contributor

@archang19 archang19 commented Dec 16, 2024

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

@hx235
Copy link
Contributor

hx235 commented Dec 16, 2024

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?

@archang19 archang19 force-pushed the suggest-destructor-override branch 3 times, most recently from bd6ae86 to d3729e9 Compare December 16, 2024 21:48
@archang19 archang19 force-pushed the suggest-destructor-override branch from d3729e9 to e46727f Compare December 16, 2024 21:52
@archang19 archang19 force-pushed the suggest-destructor-override branch from f29c2c2 to f9cce94 Compare December 16, 2024 23:55
@archang19 archang19 force-pushed the suggest-destructor-override branch from f9cce94 to 51769c4 Compare December 16, 2024 23:57
@archang19
Copy link
Contributor Author

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?

Yes we do need to update a bunch of destructors. "Quite some places" is an understatement

@archang19 archang19 force-pushed the suggest-destructor-override branch from a14b6b3 to 378d36a Compare December 17, 2024 18:28
@pdillinger
Copy link
Contributor

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.
This could be checked under the %.h.pub rule in the Makefile, but that doesn’t solve the clang version problem. We do have one warning compatibility check in build_detect_platform and could add this one, probably with a new Make/shell variable getting plumbed through.
See also #12319 re: recommended removing 'virtual' when redundant with 'override'.
Given the complications, it's also OK to drop this, or make a note to add it when the minimum clang version to support our supported C++ version includes support for the warning.

facebook-github-bot pushed a commit that referenced this pull request Dec 19, 2024
Summary:
I found this mismatch between the CI job title and the actual command ran incidentally while trying to work on #13213.

`build-linux-clang10-mini-tsan` was added in #7122 with `clang-10`.

In #10496 it was changed to use `clang-13` but the name was not also updated. I do not know what the author's intent was, but given that `build-linux-clang10-mini-tsan` is right next to`build-linux-clang10-ubsan` and `build-linux-clang10-asan`, I think it is more likely we originally intended to use `clang-10`.

Pull Request resolved: #13220

Test Plan: I think we need to wait for the next set of CI checks after this PR is merged, since I don't see my changes incorporated into this PR's `build-linux-clang10-mini-tsan` check.

Reviewed By: hx235

Differential Revision: D67407034

Pulled By: archang19

fbshipit-source-id: 9c22b6c6c330a367920eb3d4a387f37b760d722c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants