RFR [14] 8234746: Improve indexing of system properties

[Hannes Wallnöfer]

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 <pavel.rappo at oracle.com>:
> 
> 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 <jonathan.gibbons at oracle.com> 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
>> 
>> 
>> 
>> 
>> 
>> 
>