JDK 9 RFR of JDK-8162817: Annotation toString output not reusable for source input

[joe darcy]

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 
>