Page MenuHomePhabricator

Enable -Wsuggest-override in the LLVM build
ClosedPublic

Authored by logan-5 on Jul 19 2020, 9:57 AM.

Details

Reviewers
dblaikie
rnk
aaron.ballman
ldionne
lebedev.ri
Group Reviewers
Restricted Project
Restricted Project
Commits
rG8b16e45f66e2: Enable -Wsuggest-override in the LLVM build
Summary

This patch adds Clang's new (and GCC's old) -Wsuggest-override to the warning flags for the LLVM build. The warning is a stronger form of -Winconsistent-missing-override which warns _everywhere_ that override is missing, not just in places where it's inconsistent within a class.

Some directories in the monorepo need the warning disabled for compatibility's, or sanity's, sake; in particular, libcxx/libcxxabi (I've added @ldionne as a reviewer to confirm this), and any code implementing or interoperating with googletest, googlemock, or google benchmark (which do not themselves use override). This patch adds -Wno-suggest-override to the relevant CMakeLists.txt's to accomplish this.

Diff Detail

Event Timeline

logan-5 created this revision.Jul 19 2020, 9:57 AM
Herald added projects: Restricted Project, Restricted Project, Restricted Project, Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne accepted this revision.Jul 20 2020, 5:29 AM
This revision is now accepted and ready to land.Jul 20 2020, 5:29 AM
This revision was automatically updated to reflect the committed changes.

There is a lot of instances where this warning fires in the unit tests since gtest does not appear to use the override keyword. This in turn causes failures on bootstrap builds that use -Werror.
An example: http://lab.llvm.org:8011/builders/ppc64le-lld-multistage-test/builds/10717

Can we either get this pulled until the failures in bootstrap are fixed or quickly fix the code that causes the failures?

There is a lot of instances where this warning fires in the unit tests since gtest does not appear to use the override keyword. This in turn causes failures on bootstrap builds that use -Werror.
An example: http://lab.llvm.org:8011/builders/ppc64le-lld-multistage-test/builds/10717

Can we either get this pulled until the failures in bootstrap are fixed or quickly fix the code that causes the failures?

I've got fixes for these failures waiting on review (https://reviews.llvm.org/D84244). Perhaps I'll go ahead and commit them now, then revert them and fix it differently if the reviewers request it.

All the failures related to this patch are now resolved as far as I can tell. I'm very sorry for the trouble.

Ive got a bit a of a problem here: This flag (suggest-override) on GCC 8.3 (and others) warns on missing-override EVEN WHEN the function is marked 'final'. This was not fixed until GCC 9.2: https://godbolt.org/z/55KeM3

The result is my build gave a few thousand diagnostics for this. I'd suggest disabling this unless the GCC version is >9.1.

I'd suggest disabling this unless the GCC version is >9.1.

I've got a patch addressing exactly this issue here, hoping to get it landed soon: https://reviews.llvm.org/D84292

Thanks again everyone for your patience. After this experience, I can promise next time I do anything like this, it'll go much more smoothly.

labath added a subscriber: labath.Jul 24 2020, 12:47 AM

Instead of adding -Wno-suggest-override to every user it might be cleaner to add the flag to the INTERFACE_COMPILE_OPTIONS property of the relevant targets (gtest, gmock, and gbenchmark ?). That way, anything which links against these will inherit it automatically.

Instead of adding -Wno-suggest-override to every user it might be cleaner to add the flag to the INTERFACE_COMPILE_OPTIONS property of the relevant targets (gtest, gmock, and gbenchmark ?). That way, anything which links against these will inherit it automatically.

Thanks, that's a good idea -- I'll look into that as a follow-up patch.