This is an archive of the discontinued LLVM Phabricator instance.

Use INTERFACE_COMPILE_OPTIONS to disable -Wsuggest-override for any target that links to gtest
ClosedPublic

Authored by logan-5 on Jul 24 2020, 1:23 PM.

Details

Summary

This cleans up several CMakeLists.txt's where -Wno-suggest-override was manually specified. These test targets now inherit this flag from the gtest target.

Some unittests CMakeLists.txt's, in particular Flang and LLDB, are not touched by this patch. Flang manually adds the gtest sources itself in some configurations, rather than linking to LLVM's gtest target, so this fix would be insufficient to cover those cases. Similarly, LLDB has subdirectories that manually add the gtest headers to their include path without linking to the gtest target, so those subdirectories still need -Wno-suggest-override to be manually specified to compile without warnings.

Diff Detail

Event Timeline

logan-5 created this revision.Jul 24 2020, 1:23 PM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald Transcript
labath accepted this revision.Jul 27 2020, 12:45 AM

This looks good to me. Thanks.

Could you elaborate on the lldb issue? I'd like to take a look at that...

This revision is now accepted and ready to land.Jul 27 2020, 12:45 AM

Could you elaborate on the lldb issue? I'd like to take a look at that...

It looks like lldb/unittests/TestingSupport/CMakeLists.txt and lldb/unittests/TestingSupport/Symbol/CMakeLists.txt both both manually pull in the googletest/googlemock headers via include_directories. It also seems, although I'm not sure, like they don't themselves link to gtest, since simply removing those include_directories didn't work.

This revision was automatically updated to reflect the committed changes.

Could you elaborate on the lldb issue? I'd like to take a look at that...

It looks like lldb/unittests/TestingSupport/CMakeLists.txt and lldb/unittests/TestingSupport/Symbol/CMakeLists.txt both both manually pull in the googletest/googlemock headers via include_directories. It also seems, although I'm not sure, like they don't themselves link to gtest, since simply removing those include_directories didn't work.

Thanks. I think I understand the problem now. Those libraries should be linking against gtest, but they don't do that because they could get away with not doing it. And they add include directories manually because they do not get that from linking against gtest. That means we have more things to fix => D84748.