Hi Jamil,
Thanks for the code review.
Okay, I will go ahead with the comment "fatal() always throws, make the
compiler happy" for this update, and file a new bug and see if we could
make an improvement in the future.
Thanks,
Xuelei
On 12/14/2018 4:16 PM, Jamil Nimeh wrote:
Hi Xuelei, thanks for the clarification. If you want to keep the
structure as-is, then the comment "fatal() always throws, make the
compiler happy" is fine. I think that's a little more helpful to a
future maintainer who might be new to the code.
Thanks,
--Jamil
On 12/14/2018 1:56 PM, Xue-Lei Fan wrote:
On 12/14/2018 1:46 PM, Xue-Lei Fan wrote:
Hi,
The purpose of combination of the two lines together:
shc.conContext.fatal(Alert.UNEXPECTED_MESSAGE, "...");
return null; // make the complier happy
is actually for the reading of the code. If one don't know the
fatal() always throw an exception (compiler is one of them), he may
continue reading the following code, and may be confusing about the
code logic.
The "return" statement could never be executed actually. It could be
easier to catch the idea if using a format like:
throw new SSLException("there is an alert")
- return null;
But we need the fatal() method to wrap something more.
We used a lot comments similar to:
return; // fatal() always throws, make the compiler happy.
and the abbreviate comment:
return; // make the compiler happy
misses the part "fatal() always throws", and then it may look weird.
I'm fine to remove the comment, or use the full comment instead
"fatal() always throws, make the compiler happy". Which one is your
preference?
On 12/14/2018 11:49 AM, Jamil Nimeh wrote:
Looks pretty good. I did have one question about a few of the
methods in KeyShareExtension and PreSharedKeyExtension, specifically
where you return nulls on error branches with the
make-compiler-happy comments. In those cases would it be a bit
cleaner to set a byte[] variable to null at the beginning of the
method and then in the successful code path set the value to
whatever byte array comes back from hmacs or hkdf operations? At
the end of the method you only need one return statement, rather
than a return null on every error branch, many of which won't be
executed due to method calls which always throw exceptions.
Hm, I see your points. As the return statement could never be
executed, it may not worthy to declare the byte[] variable earlier.
I mean that we can remove the "return" statement as it is never
executed, if not for code reading or compiler.
In some other places, the compiler may not happy because it cannot
tell that the fatal() throws exception. So we may still need the
"return" statement a little bit. But I think most of them could be
removed if we don't like it.
Xuelei
See above. Is it a reasonable coding style to you as well?
Thanks,
Xuelei
Not a big deal if you wish to leave things as they are since I know
the current approach is used in many places in the code that this
review doesn't touch.
--Jamil
On 12/14/2018 8:14 AM, Xue-Lei Fan wrote:
Hi,
Please review the update:
http://cr.openjdk.java.net/~xuelei/8214339/webrev.00/
In some cases, the SSLProtocolException or SSLHandshakeException
may be thrown if the underlying socket run into problems. An
application may depends on the exception class for further action,
for example retry the connection with different parameters.
This update is trying to separate the socket problem from the TLS
protocol or handshake problem, by using different exception classes.
Thanks,
Xuelei
