Clean up all warnings #10
Conversation
| return v.DefaultValue(this, p); | ||
| } | ||
|
|
||
| #pragma warning disable CS0108 |
There was a problem hiding this comment.
This file is all generated code. We could have this warning disabled for all class headers. Would that be OK?
There was a problem hiding this comment.
This is due to it it being declared on interface type and CS inheriting from J, as the definition exists on both J and CS. It is essentially complaining that you defined the same class type on both parent interface and child interface. While this is syntactically allowed, it is discouraged and generally you need to use special syntax when you wanna do this deliberately. Interestingly enough MSBuild flags this as a warning (asking for new keyword to be used to ensure this is deliberate) BUT Rider is the opposite, says new keyword is not needed. Since I couldn't reconcile this behavior I just chose to disable it. I don't want to disable it at file level as it will eliminate this check for not just types but members.
I think it requires discussion on best way to handle it.
| prolog = prolog.WithMisc(ListUtils.Map(prolog.Misc, el => (Misc?)Visit(el, p))); | ||
| prolog = prolog.WithJspDirectives(ListUtils.Map(prolog.JspDirectives, el => (Xml.JspDirective?)Visit(el, p))); | ||
| prolog = prolog.WithMisc(prolog.Misc.Map(el => (Misc?)Visit(el, p))); | ||
| prolog = prolog.WithJspDirectives(prolog.JspDirectives.Map(el => (Xml.JspDirective?)Visit(el, p))); |
There was a problem hiding this comment.
I only saw you change the use of ListUtils.Map() calls in a few other places.
Since this class (and all other visitors) are generated, we would need some kind of consistent pattern we can use. Also, the thing regarding referential equality on the list when the mapping function doesn't change any elements is really important to preserve.
There was a problem hiding this comment.
I didn't change behavior of that method beyond allowing it to be used as extension method and ensuring it doesn't allow nulls to be passed in (since you were returning it as such anyways but return type was non-nullable). See change. . While the method itself doesn't allow change, when used as extension method we can still call it with ?. syntax to skip it's invocation altogether and propagate null forward at the call site, allowing us to maintain null checking rules. The extension method syntax should be used across the board as it's more concise and helps solve this null check issue. We will need to modify the generator
macsux
force-pushed
the
fix/warnings-cleanup
branch
from
c6ea3e7 to
a3b5719
Compare
3 participants
Clean up all warnings, adjust nullability across solution, treat future warnings as errors