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
