OK, modulo the formatting I have no further comments.
Some day we may revisit (some of) the code that was tricky to re-write ..
-phil.
On 2/7/2014 12:30 AM, Joe Darcy wrote:
Hello,
On 02/06/2014 04:13 PM, Phil Race wrote:
Comments/questions on the changes :-
Font2D.java
108 protected Reference<FontStrike> lastFontStrike = new
SoftReference<>(null);
...
320 lastFontStrike = new SoftReference<>(strike);
So the diamond operator can be used in this 2nd case ? Apparently so.
Yes; I verified the modified code successfully compiled :-)
I vaguely remember that it used to only be able to infer the type in
the first case ?
Did it get updated or did I just not realise ??
In JDK 7, the compiler could often infer the right type for a diamond
in most contexts, but in JDK 8, the inference scheme is more powerful.
IIRC, the main difference is that in JDK 8 the compiler can do a
better job of inferring diamond when it occurs in a position like an
argument in a method call (where there is an interaction between the
inference of diamond and the overload resolution logic).
There are a few places including these :-
-----
AttributeValues.java
Map<TextAttribute, ?> imStyles = hl.getStyle();
FontResolver.java
226 public Font getFont(int index,
227 Map<? extends
AttributedCharacterIterator.Attribute, ?> attributes) {
----
where its not obvious how you decided the type to use.
In that particular case, I looked at the return type of the method
java.awt.Font.getAttributes(); I did the analogous looking in the
other case.
I know a lot of this code and that they look reasonable but I
wouldn't have been confident
that there weren't places where we allowed some wider type to be used
in these maps.
How did you decide on the types and how did you verify it ?
The verification occurs by compiling the JDK; if there was a mismatch
the compiler would compile. (Since these are sun.* classes, they are
officially only supposed to be used inside the JDK.)
@SuppressWarnings("unchecked")
Can you show what thevarious warnings looked like and say why they
needed to be suppressed rather than fixed ?
In AttributeValues, the warning in the original code is:
src/share/classes/sun/font/AttributeValues.java:822: warning:
[unchecked] unchecked cast
av = AttributeValues.fromMap((Map<Attribute, ?>)map);
// yuck
^
required: Map<Attribute,?>
found: Map<CAP#1,CAP#2>
where CAP#1,CAP#2 are fresh type-variables:
CAP#1 extends Object from capture of ?
CAP#2 extends Object from capture of ?
The map value comes in as a parameter of type Map<?, ?> map and then
some runtime check verify it is an AttributeMap. This these two
unchecked casts are isolated, I didn't think it was worthwhile to try
to perform more extensive surgery to try to fix up the types.
Is it anywhere you found it to tricky/impossible to get rid of a cast ?
Yes, largely where @SuppressWarning("unchecked") was used :-) One
example, in
+ @SuppressWarnings("unchecked")
HashMap<String,String> ffmapCopy =
(HashMap<String,String>)(fontToFileMap.clone());
the unchecked suppression is needed because the clone method in
HashMap is declared to return "Object" rather than "HashMap<K, V>"
even though it does actually return a "HashMap<K, V>". I looked into
changing this in collections, but there are subtle compatibility
issues that unfortunately argue for leaving these particular clone
methods as returning Object.
There are a few occurrences of casting the result of cloning a
collection.
The appContext mechanism is not fully generics friendly and needed
some casts and unchecked suppression.
Some code in SunFontManager.java attempted to make generics and arrays
play together in an unnatural way and @SuppressWarning("unchecked")
was needed to keep the peace without attempting a larger rewrite.
FontScaler.java :
Is the whole somewhat ugly re-writing of this so that you have a
single expression
to which to apply @SuppressWarnings ?
Basically, but the situation is worse than usual since Class.forName
maps a String to a Class<?> and I wanted only one cast to (Class<?
extends FontScaler>) with a single @SuppressWarnings for that cast.
If you are going to do that anyway was it worth it ? Can't it just
be applied
to the static initialiser block ?
The SuppressWarning declaration cannot be applied to the static
initializer block since the block is not the sort of declaration that
can host an annotation.
And at the very least there should be spaces before "?" and ":" to
make it
easier to read.
Okay.
StrikeCache.java:
Disposer.addReference((Reference<Object>)(Reference)this, disposer);
Why does this look like a cast of an already cast variable ? Looks
weird to me.
The short answer is that a cast through a raw type, "Reference" in
this case, is sometimes necessary to convince the compiler you can do
something it doesn't think you should be able to do. The "this" value
has a type of Reference<FontStrike> and that is not directly
convertible to Reference<Object>. (The Disposer is weirdly typed,
basically a type-hetrogenous container, and would have to be more
extensively revised to play well with generics.)
SunFontManager.java
lines 2310-2314 look much > 80 chars. We keep all these files <=80
chars wide
I find it tricky to catch all those in review but please be conscious
of it.
I suspect line 2644 in the same file is also too long.Also 3066,
3068, 3128, 3157..
please fix all of these and any others I may have overlooked in this
or other files.
Okay.
And don't forget - please re-generate against client, or ask me to
push there for you.
-phil.
Thanks for the review,
-Joe
