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

objc: fix envoy_observer native_init #337

Merged
merged 2 commits into from
Aug 16, 2019
Merged

objc: fix envoy_observer native_init #337

merged 2 commits into from
Aug 16, 2019

Conversation

rebello95
Copy link
Contributor

This was being instantiated in the wrong order, and warning at compile time that the wrong closure types were being assigned to envoy_observer:

library/objective-c/EnvoyEngine.m:228:33: warning: incompatible pointer types initializing 'envoy_on_error_f' (aka 'void (*)(envoy_error, void *)') with an expression of type 'void (void *)' [-Wincompatible-pointer-types]
                                ios_on_complete, ios_on_error, context};
                                ^~~~~~~~~~~~~~~
library/objective-c/EnvoyEngine.m:228:50: warning: incompatible pointer types initializing 'envoy_on_complete_f' (aka 'void (*)(void *)') with an expression of type 'void (envoy_error, void *)' [-Wincompatible-pointer-types]
                                ios_on_complete, ios_on_error, context};
                                                 ^~~~~~~~~~~~

Signed-off-by: Michael Rebello [email protected]

This was being instantiated in the wrong order, and warning at compile time that the wrong closure types were being assigned to `envoy_observer`:

```
library/objective-c/EnvoyEngine.m:228:33: warning: incompatible pointer types initializing 'envoy_on_error_f' (aka 'void (*)(envoy_error, void *)') with an expression of type 'void (void *)' [-Wincompatible-pointer-types]
                                ios_on_complete, ios_on_error, context};
                                ^~~~~~~~~~~~~~~
library/objective-c/EnvoyEngine.m:228:50: warning: incompatible pointer types initializing 'envoy_on_complete_f' (aka 'void (*)(void *)') with an expression of type 'void (envoy_error, void *)' [-Wincompatible-pointer-types]
                                ios_on_complete, ios_on_error, context};
                                                 ^~~~~~~~~~~~
```

Signed-off-by: Michael Rebello <[email protected]>
@rebello95 rebello95 requested review from goaway and junr03 August 16, 2019 04:31
keith
keith previously approved these changes Aug 16, 2019
Copy link
Member

@keith keith left a comment

Choose a reason for hiding this comment

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

You should turn on warnings as errors on CI. You can with --copt=-Werror. After that you might want to tune and enable more warnings as well. If this is too much because of envoy you could also enable it just for this project's cc_library targets.

Signed-off-by: Michael Rebello <[email protected]>
@rebello95
Copy link
Contributor Author

@keith that's a great callout. Tried running locally with warnings as errors, but as you mentioned it fails with core Envoy. What would be the best way to run this only on specific Envoy Mobile cc_library targets?

Copy link
Contributor

@goaway goaway left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for catching this!

Per discussion here and elsewhere, I think @keith's suggestion of running with -Werror is a good one, and I like @junr03's idea of creating a project-specific bazel wrapper rule that adds it (at least for now).

@rebello95
Copy link
Contributor Author

Filed #339

@rebello95 rebello95 merged commit 65652c5 into master Aug 16, 2019
@rebello95 rebello95 deleted the fix-objc-callbacks branch August 16, 2019 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants