Hi Pavel, The changes look good to me.
I just realised that I'll have to adapt my fix for JDK-8235414 in order to stay consistent with this change: https://mail.openjdk.java.net/pipermail/javadoc-dev/2019-December/001222.html This sets the holder of index tags/system properties defined in doc-files depending on whether the doc-files are at package or module level. To remain consistent with your change we should prepend „package“ or „module“ in that case as well, which I think is a good idea anyway. I think it’s ok move forward with your patch and then adapt my patch to match it, unless Jon has some concern. Hannes > Am 09.12.2019 um 16:30 schrieb Pavel Rappo <[email protected]>: > > Hey Jon, thanks for looking at it. I have updated the webrev. This new webrev > addresses Hannes' and yours comments: > > http://cr.openjdk.java.net/~prappo/8234746/webrev.01/ > > -Pavel > >> On 27 Nov 2019, at 21:20, Jonathan Gibbons <[email protected]> >> wrote: >> >> >> >> >> On 11/25/2019 09:19 AM, Pavel Rappo wrote: >>> Hello, >>> >>> Please review the following change for >>> https://bugs.openjdk.java.net/browse/JDK-8234746 >>> : >>> >>> >>> http://cr.openjdk.java.net/~prappo/8234746/webrev.00/ >>> >>> >>> Before: >>> >>> javax.rmi.ssl.client.enabledCipherSuites - Search tag in >>> javax.rmi.ssl.SslRMIClientSocketFactory >>> javax.rmi.ssl.client.enabledCipherSuites - Search tag in >>> javax.rmi.ssl.SslRMIClientSocketFactory >>> ... >>> file.separator - Search tag in java.lang.System >>> >>> After: >>> >>> javax.rmi.ssl.client.enabledCipherSuites - Search tag in >>> javax.rmi.ssl.SslRMIClientSocketFactory >>> javax.rmi.ssl.client.enabledCipherSuites - Search tag in >>> javax.rmi.ssl.SslRMIClientSocketFactory.createSocket >>> ... >>> file.separator - Search tag in java.lang.System.getProperties >>> >>> Thanks, >>> -Pavel >>> >>> >> >> You either need a test or mark the bug as noreg-something. >> >> >> Uugh. >> >> I agree that the fix will work, but I cringe at the methods in Utils that >> make you fix it this way. >> >> Readingline 441 of your fix, it is "obviously wrong" since the simple name >> of an element is generally the last component of the qualified name, but I >> see that "Utils.getFullyQualifiedName" will (and is specified to) return the >> qualified name of the enclosing element in some cases, which is not an >> intuitive definition based on the method's name. >> >> I guess I wonder how often Utils.getFullyQualifiedName is invoked with a >> field or method, and whether we should make those use sites use >> Utils.getFullyQualifiedName(e.getEnclosingElement()) if they really want the >> qualified name of the enclosing element, so that >> Utils.getFullyQualifiedName() can throw ILA if called with an element that >> does not have a fully qualified name. >> >> Proactively applying that logic to your fix, it would be less jarring if you >> were to change it to: >> - 441 si.setHolder(utils.getFullyQualifiedName(e) + >> "." + utils.getSimpleName(e)); >> >> + 441 >> si.setHolder(utils.getFullyQualifiedName(e.getEnclosingElement()) + "." + >> utils.getSimpleName(e)); >> >> That being said, I see your fix is the same as in line 435, which suggests >> we might want to modify that line as well to better reflect the notions of >> fully qualified name and simple name. Curiously, the code for visitVariable >> "almost" gets it right, by getting the enclosing type element (te) but then >> it ignores it. >> >> --Jon >> >> >> >> >> >> >
