[generator] fix C# compiler warning in every Invoker #217
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Context: https://github.com/xamarin/AndroidSupportComponents When compiling the Xamarin support libraries from source, its binding projects generate thousands of warnings. Most of these warnings are noise that could be fixed, and `generator` is just overdue for some improvement. My first steps to improve this: - By default `generator-Tests` should fail if there is a C# compiler warning - Add an `AllowWarnings` option for `BaseGeneratorTest`, since there are a few tests that generate C# compiler warnings that aren’t addressed yet. The hope is this feature could be dropped from the tests eventually. - Fix the most common warning: `Invoker` types have a warning of a missing `new` keyword on the generated `class_ref` field Since `Java.Lang.Object` has a static `class_ref` property, all `Invoker` types will produce this warning. An `Invoker` is generated for any `interface` or `abstract` class, so you can see how a large binding project could run into this warning many times.
|
@jonathanpeppers, |
jonathanpeppers
added a commit
to jonathanpeppers/java.interop
that referenced
this pull request
Dec 13, 2017
Context: dotnet#217 There is one difference in generator-Tests compared to how real Xamarin.Android binding projects would work: - generator-Tests put everything in one assembly - Real binding projects have `Java.Lang.*` in a separate assembly This means that some members that are marked `internal` will not need the `new` keyword at all—even though the tests currently *would* need the `new` keyword. The real fix here is to make the tests generate two separate assemblies so that we are more closely reproducing what real Xamarin.Android binding projects would do. Since we are branching for 15-6 soon, it seems better to revert the `new` keyword addition, and set `AllowWarnings=true` in some of these tests. I also included a comment for every test that sets `AllowWarnings=true`, so that it is clear which warning is occurring.
jonathanpeppers
added a commit
to jonathanpeppers/java.interop
that referenced
this pull request
Dec 13, 2017
Context: dotnet#217 When testing generator from xamarin-android/master, I noticed that building a binding project has some *new* warnings: ``` obj/Debug/generated/src/GoogleAnalytics.Tracking.ILogger.cs(159,14): warning CS0109: The member 'ILoggerInvoker.class_ref' does not hide an accessible member. The new keyword is not required. ``` Binding project: https://github.com/jonathanpeppers/GoogleAnalytics/tree/master/GoogleAnalytics.Droid This was due to a change made in dotnet#217, I was *tricked* by how warnings operate differently in `generator-Tests` than what happens in a real Xamarin.Android binding project. The difference is: - `generator-Tests` put everything in one assembly - Real binding projects have `Java.Lang.*` and Android types in a separate assembly This means that some members that are marked `internal` will not need the `new` keyword at all—even though the tests currently *would* need the `new` keyword. The real fix here is to make the tests generate two separate assemblies so that we are more closely reproducing the situation of a Xamarin.Android binding project. Since we are branching for 15-6 soon, it seems better to revert the `new` keyword change in `InterfaceGen`, and set `AllowWarnings=true` where required in `generator-Tests`.. I also included a comment for every test that sets `AllowWarnings=true`, so that it is clear which warning is occurring.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context: https://github.com/xamarin/AndroidSupportComponents
When compiling the Xamarin support libraries from source, its binding
projects generate thousands of warnings. Most of these warnings are
noise that could be fixed, and
generatoris just overdue for someimprovement.
My first steps to improve this:
generator-Testsshould fail if there is a C# compilerwarning
AllowWarningsoption forBaseGeneratorTest, since thereare a few tests that generate C# compiler warnings that aren’t
addressed yet. The hope is this feature could be dropped from the tests
eventually.
Invokertypes have a warning of amissing
newkeyword on the generatedclass_reffieldSince
Java.Lang.Objecthas a staticclass_refproperty, allInvokertypes will produce this warning. An
Invokeris generated for anyinterfaceorabstractclass, so you can see how a large bindingproject could run into this warning many times.