Code Review Request JDK-8215443: The use of TransportContext.fatal() leads to bad coding style

[Anthony Scarpino]

On 12/17/18 5:26 PM, Xue-Lei Fan wrote:
> On 12/17/2018 1:17 PM, Anthony Scarpino wrote:
>> It looks like in TransportContext.java:68, you had a mistype that 
>> added "fa" to the end of a comment.
>>
> Oops, I will update it.
> 
>> Also in fatal():267, did you plan to return the exception and have the 
>> calling method throw the exception?  As is, the exception is never 
>> return and fatal() continues to throw the exceptions.
>>
> I considered to return the exception.  I'm not very confident with if I 
> searched out all use of the fatal() methods.  For safe, I kept to use 
> thrown exception instead. If the method is updated later that there is a 
> case that now exception get thrown, there will be a compiling issue. Are 
> you OK with it?

Since the stacktrace is based on the exception's creation and not where 
it's thrown, I guess there's no reason to return the value.  As you have 
it now should be fine.

Tony

> 
> Thanks,
> Xuelei
> 
>> Tony
>>
>> On 12/15/18 7:51 AM, Xue-Lei Fan wrote:
>>> Hi,
>>>
>>> Could I have the update reviewed?
>>>     http://cr.openjdk.java.net/~xuelei/8215443/webrev.00/
>>>
>>> The TransportContext.fatal() methods always throw exception. While 
>>> the compiler does not aware of it, and may not happy without 
>>> following a return statement.  Currently, a lot never executable 
>>> return statements are inserted.  As make the code hard to read 
>>> (thanks for Jamil and Tony's points).  For example:
>>>
>>>      shc.conContext.fatal(Alert.UNEXPECTED_MESSAGE, ...);
>>>      return null;    // fatal() always throws, make the compiler happy.
>>>
>>> In this update, I changed the fatal() method with a return value:
>>>
>>> -    void fatal(Alert alert, ...
>>> +    SSLException fatal(Alert alert, ...
>>>
>>> Then we can change the use of method as:
>>>
>>> -    shc.conContext.fatal(Alert.UNEXPECTED_MESSAGE, ...);
>>> -    return null;    // fatal() always throws, make the compiler happy.
>>> +    throw shc.conContext.fatal(Alert.UNEXPECTED_MESSAGE, ...);
>>>
>>> The changeset is mostly about removing the never executed return 
>>> statements and add the 'throw' keyword to lines that use the fatal() 
>>> methods.
>>>
>>> Thanks,
>>> Xuelei
>>