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