RFR: JDK-8223914: specification of j.l.c.MethodTypeDesc::of should document better the exceptions thrown

[Roger Riggs]

Hi Vicente,

Looks ok.

Roger


On 05/20/2019 03:29 PM, Vicente Romero wrote:
> Hi Roger,
>
> On 5/20/19 2:00 PM, Roger Riggs wrote:
>> Hi Vicente,
>>
>> In the CSR, the Summary should be about the change... 
>> "...MethodTypeDesc::of should document all exceptions.
>> Avoid duplication between Summary and Problem.
>
> thanks, I saw that you already modified the Summary
>>
>> I would omit the part about "content of parameter" or "its contents" 
>> is null;
>> It cannot happen and if it does, its more of an internal error than a 
>> regular NPE
>> since it should not be possible to create a ClassDesc with a null 
>> descriptor string.
>
> this spec comment was included in this one an other similar methods. 
> It is probably a bit of an overkill but we have already changed the 
> spec of several similar methods to this pattern, see for example [1]
Its just a bad to propagate a poor spec as it is to write new bad spec.

>
>>
>> In the Code Review:
>>
>> MethodTypeDesc.java: 68:  as above, "or its contents are" -> "is "  
>> there's no need to mention the contents.
>>
>> MethodTypeDescTest.java: 264 and 274.  The messages would be more 
>> useful (if they ever were to happen)
>> and for the person reading the code if they describe what should not 
>> happen.
>> For example,  "ClassDesc array should not be null"  or ClassDesc 
>> should not be null.
>
> I made this change in the test comment [2]
>>
>> Thanks, Roger
>
> Thanks,
> Vicente
>
> [1] https://bugs.openjdk.java.net/browse/JDK-8223803
> [2] http://cr.openjdk.java.net/~vromero/8223914/webrev.01/
>
>>
>>
>> On 05/17/2019 12:55 PM, Vicente Romero wrote:
>>> Please review simple fix for [1] at [2] plus the CSR at [3]. This 
>>> fix is simply documenting all the missing cases in which method 
>>> java.lang.constant.MethodTypeDesc::of can throw exceptions. A test 
>>> has been added to cover the missing cases.
>>>
>>> Thanks,
>>> Vicente
>>>
>>> [1] https://bugs.openjdk.java.net/browse/JDK-8223914
>>> [2] http://cr.openjdk.java.net/~vromero/8223914/webrev.00/
>>> [3] https://bugs.openjdk.java.net/browse/JDK-8224136
>>
>