RFR [14] 8234746: Improve indexing of system properties

[Pavel Rappo]

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
> 
> 
> 
> 
> 
>