[OpenJDK 2D-Dev] OpenJDK Warning hackathon :) - patch 1

[Jim Graham]

Phil described my take on it pretty well, but I do see your point about 
hiding where they come from.

If I had to do it over again I might put them into an interface that 
SG2D, the SD's and other pipeline objects could all inherit with an 
"implements" since the constants are so intimately shared.  Josh Bloch 
has spoken against that usage, though, since the constants then become 
part of the API of all of those classes, but in our case this is 
internal API so we don't have to worry about binary compatibility 
outside of a given release.

If we did it that way, that would likely be beyond the scope of this 
current exercise since the goal is to not change any APIs (even internal 
ones) to isolate the warnings changes (as simple as this change is).  On 
the other hand, other fixes involve declaring more specific types for 
internal generified objects which also changes API, so this is a fuzzy 
line in the sand.

Interestingly, both a static import of * and moving the constants to a 
shared interface would both result in the constants simply being 
referenced without a qualifier.

Finally, I have had, in the back of my head, the thought that we should 
eventually try to move all of the constants into a single field as much 
as possible since they bloat the SG2D object by so much which just 
wastes memory and cycles to create and clone them.  Doing that would 
likely require a lot of bitmasking which would likely lead to a helper 
class which would be a completely different rewrite of all of this code.

I'm happy to approve whichever version Mario wants to promote (except 
the rewrite to bitfields which is clearly outside the scope here) since 
he is doing the work though...

			...jim

On 12/2/2011 12:07 PM, Phil Race wrote:
> I'll leave it to Jim to decide if he still would like the static imports.
> I do see his point that SurfaceData and SG2D are so inter-related the
> statics
> could almost as easily be on the SD class so it would look neater to be
> able
> to use them as if it were that way.
>
> If you aren't sure that HashMap<String, int[]> is OK then do leave it alone
> since erring on the side of caution is one of the rules for this exercise.
> But maybe Jim can be more sure about this for you.
>
> So I'm OK with the rest of changes and however you and Jim resolve the
> above issues.
>
> -phil.
>
> On 12/2/2011 6:58 AM, Mario Torre wrote:
>> 2011/12/2 Jim Graham<james.graham at oracle.com>:
>>> For GraphicsPrimitive, can't we determine a decent specific type for the
>>> traceMap to avoid having to suppress the warnings?
>>>
>>> And looking through the rest of the changes, I think I'd support an
>>> SG2D.*
>>> static import in any of the pipelines and loops as well...
>>>
>>>
>>> ...jim
>>>
>> Ok, I reverted the includes, and I fixed the Path2D with the small
>> duplication
>> rather than the suppress warning, actually I think is indeed better
>> that way.
>>
>> I don't fully agree with the static import, since they tend to hyde
>> the scope of
>> the class rather than make the code more readable (well, that's my
>> opinion at
>> least), I rather prefer to keep things explicit, but if you want I can
>> prepare
>> another webrev with the static imports of course.
>>
>> The map key seems to always be either a GraphicsPrimitive of some kind
>> or a
>> String, while the value should always be an integer array.
>>
>> This map is package private, although I didn't find it used elsewhere
>> (even a grep in native code) so probably the wildcard will do just fine,
>> I would leave it like this to be honest for now.
>>
>> Anyway, this is the latest webrev:
>>
>> http://cr.openjdk.java.net/~neugens/warning-hackaton/webrev.04/
>>
>> Cheers,
>> Mario
>