Hi Peter,
On 8/2/2016 1:59 AM, Peter Levart wrote:
Hi Joe,
I wonder why you compare the type obtained from an value.getClass()
with class literals for primitive types (lines 174, 176, 178):
165 Class<?> type = value.getClass();
166 if (!type.isArray()) {
167 // primitive value, string, class, enum const, or
annotation
168 if (type == Class.class)
169 return toSourceString((Class<?>) value);
170 else if (type == String.class)
171 return toSourceString((String) value);
172 if (type == Character.class)
173 return toSourceString((char) value);
174 else if (type == double.class)
175 return toSourceString((double) value);
176 else if (type == float.class)
177 return toSourceString((float) value);
178 else if (type == long.class)
179 return toSourceString((long) value);
180 else
181 return value.toString();
182 } else {
They will never match!
Indeed! Just goes to show the value of making sure there are no coverage
gaps in one's regression tests ;-) In an updated version of the patch,
http://cr.openjdk.java.net/~darcy/8162817.2/
I replaced
174 else if (type == double.class)
175 return toSourceString((double) value);
176 else if (type == float.class)
177 return toSourceString((float) value);
178 else if (type == long.class)
179 return toSourceString((long) value);
180 else
181 return value.toString();
with
174 else if (type == Double.class)
175 return toSourceString((double) value);
176 else if (type == Float.class)
177 return toSourceString((float) value);
178 else if (type == Long.class)
179 return toSourceString((long) value);
180 else
181 return value.toString();
and added a test case to check the following annotation:
76 @MostlyPrimitive(
77 c0='a',
78 c1='\'',
79 i0=1,
80 i1=2,
81 f0=1.0f,
82 f1=Float.NaN,
83 d0=0.0,
84 d1=2.0/0.0,
85 l0=5,
86 l1=Long.MAX_VALUE,
87 s0="Hello world.",
88 s1="a\"b",
89 class0=Obj[].class
90 )
Many of the new toString forms of these members differ from the current
ones.
Also, sometimes you use "else if (...)" and sometimes just "if (...)".
They are both logically correct as you always "return" in the body of
the previous if statement, but it is not very consistent...
Otherwise looks good.
The existing code in this class (most of it dating back to 2003!), was
pretty consistent in its "if {return ...} - if {return ...}" usage.
However, when there are logical alternatives, I find it marginally
clearer to use a "if {return ...} else if {return ...}. ..." structure.
(If there was switching on Class objects, this class would be a great
candidate to use it.)
However, I didn't think it was justified to update the rest of the class
to use the if-else pattern.
Thanks for the review,
-Joe
Regards, Peter
On 08/01/2016 11:39 PM, joe darcy wrote:
This change should cover 99 44/100 % of the annotation values that
appear in practice; limited efforts were taken quoting characters in
strings, etc.
The basic approach is to introduce a family of overloaded
toSourceString methods to wrap/filter different kinds of values
coupled with methods to convert the various primitive arrays to
Stream<String> for final processing by a shared method to surround an
array by "{" and "}" and add comma separators as needed.
Thanks,
-Joe
