On 2/26/18 4:23 PM, Jeremy Manson wrote:
Hi Mandy,
Thanks for taking this on! I'm happy to see that you are happy to do
cleanups I was too timid to do (like adding the Factory in the tests).
I note a few places in the test code where static initializers can
throw RuntimeExceptions. When I ran the tests, and a static
initializer threw a RuntimeException, I didn't see it reflected in the
output, so I had to add print statements. Was I just doing it wrong,
or is that a feature of jtreg?
Do you see ExceptionInInitializerError and its cause should be
RuntimeException? ExceptionInInitializerError is thrown when <clinit>
fails.
Just a couple of random nits:
> 267 * @throws IllegalArgumentException if the given
CompositeData does not
> 268 * contain all attributes representing ThreadInfo of any
release.
Could this be misinterpreted? If you have everything in V5, and only
one of the V6 attributes, it contains all of the attributes
representing the ThreadInfo of V5. Maybe you want to say "does not
contain exactly the attributes present in ThreadInfo for a particular
release"?
The javadoc has this statement:
A CompositeData representing a ThreadInfo of version N must contain
all the attributes defined since N or earlier unless specified otherwise.
What about:
@throws IAE if the given CompositeData does not contain all {@linkplain
#attrs ThreadInfo attributes} for a release.
I will add a link to the table.
For signatures like:
> static CompositeType initV5CompositeType(CompositeType ctype) {
I probably would change the name of ctype to indicate when you are
reading the method that the parameter is always supposed to be the the
CompositeType for ThreadInfo.
Let me think about it and send another webrev. I am tempted to improve
and provide an automated way for the versioning support.
Mandy
On Fri, Feb 23, 2018 at 4:50 PM, mandy chung <mandy.ch...@oracle.com
<mailto:mandy.ch...@oracle.com>> wrote:
Webrev at:
http://cr.openjdk.java.net/~mchung/jdk11/webrevs/8198253/webrev.01
<http://cr.openjdk.java.net/%7Emchung/jdk11/webrevs/8198253/webrev.01>
This patch updates the spec to clarify what attributes are required
since which release. There is a spec bug that "classLoaderName"
added in JDK 9 is missing in the CompositeData for StackTraceElement
but the implementation is correct. I will file a CSR for this update.
This patch ensures that CompositeData for ThreadInfo of version N
must have the attributes defined since <= N.
ThreadInfo::from also makes sure 'stackTrace' and 'lockedMonitors'
attributes of version N while it can include additional attributes
which has been the current behavior.
JDK-8139587 intended to support older versions of StackTraceElement
which does not seem a complete solution. I reverted the fix for
JDK-8139587 (mostly) and removed TypeVersionMapper. The fix constructs
the CompositeType for ThreadInfo and MonitorInfo of different
versions and used them for validation. Minor cleanup: the static
final variables are renamed to all capitals. CompatibilityTest.java
test is missing the copyright header.
thanks
Mandy
[1]http://cr.openjdk.java.net/~mchung/jdk11/webrevs/8198253/api/java/lang/management/ThreadInfo.html#from(javax.management.openmbean.CompositeData)
<http://cr.openjdk.java.net/%7Emchung/jdk11/webrevs/8198253/api/java/lang/management/ThreadInfo.html#from%28javax.management.openmbean.CompositeData%29>
