This is an archive of the discontinued LLVM Phabricator instance.

Disable warnings related to anonymous types in the ObjC plugin
ClosedPublic

Authored by vsk on Dec 1 2017, 4:34 PM.

Details

Summary

This part of lldb make use of anonymous structs and unions. The usage is
idiomatic and doesn't deserve a warning. Logic in the NSDictionary and NSSet
plugins use anonymous structs in a manner consistent with the relevant Apple
frameworks.

Diff Detail

Event Timeline

vsk created this revision.Dec 1 2017, 4:34 PM
davide accepted this revision.Dec 2 2017, 12:25 PM
davide added a subscriber: davide.

LGTM, I stumbled upon the same issue :)
An alternative would be that of naming (as you suggested), but if this is consistent, no worries.

This revision is now accepted and ready to land.Dec 2 2017, 12:25 PM
labath edited edge metadata.Dec 4 2017, 4:56 AM

If the "excuse" for not following llvm here is that these structs mirror apple headers, should we restrict these warnings only to the relevant files (e.g. everything in source/Plugins/Language/ObjC)?

I don't particularly care about whether we do, but I wanted to throw the idea out there, as this is the reason why I haven't done something similar yet (although the warnings are quite annoying).

vsk added a comment.Dec 4 2017, 3:01 PM

If the "excuse" for not following llvm here is that these structs mirror apple headers, should we restrict these warnings only to the relevant files (e.g. everything in source/Plugins/Language/ObjC)?

I don't particularly care about whether we do, but I wanted to throw the idea out there, as this is the reason why I haven't done something similar yet (although the warnings are quite annoying).

I'd like to make this a narrower change as well, but unfortunately, I haven't found a way to add in extra CXX flags through the add_library or llvm_add_library utilities. Somebody tried to do this once in AddLLDB.cmake (see "set(CMAKE_CXX_FLAGS ...)"), but it's busted, and does not do anything, AFAICT.

In this case, I think it should be safe to disable these warnings project-wide, because they're not often (ever?) immediate indicators of correctness issues / undefined behavior. Given all of that, @labath are you OK with the patch as-is?

cmake/modules/LLDBConfig.cmake
241

I should point out that there's a typo here -- I've fixed this so it's spelled CXX_SUPPORTS_NO_NESTED_ANON_TYPES locally.

labath accepted this revision.Dec 5 2017, 2:52 AM
In D40757#944293, @vsk wrote:

I'd like to make this a narrower change as well, but unfortunately, I haven't found a way to add in extra CXX flags through the add_library or llvm_add_library utilities. Somebody tried to do this once in AddLLDB.cmake (see "set(CMAKE_CXX_FLAGS ...)"), but it's busted, and does not do anything, AFAICT.

Yeah, I believe the correct cmake incantation in this case should be:

target_compile_options(lldbPluginObjCLanguage PRIVATE -Wno-nested-anon-types -Wno-gnu-anonymous-struct)

(placed just after add_lldb_library)

In this case, I think it should be safe to disable these warnings project-wide, because they're not often (ever?) immediate indicators of correctness issues / undefined behavior. Given all of that, @labath are you OK with the patch as-is?

I'm fine with this as well, but since we both agree that a narrower scope would be better, maybe you could try the above snippet instead.

vsk updated this revision to Diff 125816.Dec 6 2017, 2:23 PM
vsk retitled this revision from Disable warnings related to anonymous types to Disable warnings related to anonymous types in the ObjC plugin.
vsk edited the summary of this revision. (Show Details)
  • Thanks @labath! I've disabled the GNU warnings in a narrower way. PTAL.
labath accepted this revision.Dec 7 2017, 1:57 AM

Cool. Thanks.

davide added a comment.Dec 7 2017, 6:58 AM

LGTM (again)

This revision was automatically updated to reflect the committed changes.