Looks good to me.
-- Jon
On 12/09/2019 07:30 AM, Pavel Rappo wrote:
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
