Skip to content
This repository has been archived by the owner on Mar 26, 2020. It is now read-only.

Suppress -Wdirect-ivar-access in generated objcpp files #255

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mknejp
Copy link
Contributor

@mknejp mknejp commented Jul 15, 2016

We just added djinni generated files to an existing iOS project and it blessed us with about 200 new warnings. This adds #pragma clang diagnostic ignored "-Wdirect-ivar-access" to .mm files unless someone knows how to generate equally efficient code that doesn't trigger it.

@smarx
Copy link

smarx commented Jul 15, 2016

Automated message from Dropbox CLA bot

@mknejp, it looks like you've already signed the Dropbox CLA. Thanks!

@artwyman
Copy link
Contributor

What build settings does the affected project have? Is this warning turned on explicitly?
I haven't seen this warning, even though we run with -Werror -Wall -Wextra, and I don't even see an option to enable this warning in Xcode. This change isn't really dangerous, and the fact that it's ObjC++ code overcomes most of my normal concerns about the compiler-specific pragma. However I wonder whether non-default-enabled warnings should be better addressed by changing project/compile settings rather than modifying the generator.

@mknejp
Copy link
Contributor Author

mknejp commented Jul 27, 2016

The warning is turned on manually in a .xcconfig because of coding guidelines and the generated code is in the same project. Sure, the generated files could be put into a different project but that means more build settings, more dependencies, more maintenance overhead.

@artwyman
Copy link
Contributor

You can also add compile flags to individual files in a target in Xcode (though gyp can't do that).

Hmm, I'm torn. It seems like there might be a slippery slope here, since we don't want the Djinni generator to have to be modified for lots of non-standard build flags which different people come up with. I suspect a change like this might be hard to maintain and subject to accidental breakage by Djinni devs who don't run with this warning enabled, and miss a case when changing or refactoring code.

That being said, there's no risk of breaking anyone who doesn't have the flag enabled, so I can merge this. Two requests before I do:

  • Please add a comment in the generator code explaining why the pragma is there.
  • Please wrap the pragma in #ifdef clang just to avoid unknown pragma warnings in other compilers. It's unclear if anyone compiles Objective-C in any other compiler, but it is technically possible (there's a GNU ObjcC compiler, apparently).

@mknejp mknejp force-pushed the feature/Wdirect-ivar-access branch from 751885a to 3b14c7f Compare July 29, 2016 00:56
@mknejp mknejp force-pushed the feature/Wdirect-ivar-access branch from 3b14c7f to fe052be Compare July 29, 2016 02:34
@mknejp
Copy link
Contributor Author

mknejp commented Nov 5, 2016

I almost forgot this was still open. Currently looking at this again. One thing though:

Please wrap the pragma in #ifdef clang just to avoid unknown pragma warnings in other compilers. It's unclear if anyone compiles Objective-C in any other compiler, but it is technically possible (there's a GNU ObjcC compiler, apparently).

There's already __has_feature(objc_arc) in the generated files which is Clang-only so I'd say the compiler choice is pretty much set.

@@ -138,6 +138,10 @@ class ObjcppGenerator(spec: Spec) extends BaseObjcGenerator(spec) {
val objcSelf = if (i.ext.objc && i.ext.cpp) self + "CppProxy" else self

if (i.ext.cpp) {
// Disable warnings in Clang about directly accessing the ivars of an object
w.wl("#if __clang__")
w.wl("#pragma clang diagnostic ignored \"-Wdirect-ivar-access\"")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would remove the w.wl("#if __clang__") w.wl("#endif") wrapping, else lgtm. We currently suppress the warning via an xcconfig file but having a more selective suppression would be really nice. Thanks for working on that!

Copy link
Contributor

@artwyman artwyman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested changes inline. The xcodeproj files also need merging with recent changes before this can be merged.

@@ -138,6 +138,10 @@ class ObjcppGenerator(spec: Spec) extends BaseObjcGenerator(spec) {
val objcSelf = if (i.ext.objc && i.ext.cpp) self + "CppProxy" else self

if (i.ext.cpp) {
// Disable warnings in Clang about directly accessing the ivars of an object
w.wl("#if __clang__")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the indentation of the new code here is mixing tabs and spaces. Can you standardize on spaces to match the existing code, please?

// Disable warnings in Clang about directly accessing the ivars of an object
w.wl("#if __clang__")
w.wl("#pragma clang diagnostic ignored \"-Wdirect-ivar-access\"")
w.wl("#endif")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The generated code doesn't include the #if/#endif here, so I presume it wasn't regenerated after the most recent change? Please re-generate to reflect the latest state of the generator. After seeing the commentary I'm fine with merging this either with or without the #if.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants