JDK 9 RFR of JDK-8162817: Annotation toString output not reusable for source input
joe darcy
joe.darcy at oracle.com
Tue Aug 2 17:16:02 UTC 2016
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
>
More information about the core-libs-dev
mailing list