<i18n dev> Request for review: 7084245: Update usages of InternalError to use exception chaining

[Xueming Shen]

Hi Sebastian,

On 08/30/2011 01:23 AM, Sebastian Sickelmann wrote:
> Sorry i have forgotten the webrev url.
>
> http://oss-patches.24.eu/openjdk8/InternalError/part2/7084245_main_1/
>
>>
>>> with couple changes from your original patch.
>>>
>>> (1) Undo the changes in DecimalFormat.java and Format.java.
>>>      while arguably your suggested change might be the 
>>> correct/better one for
>>>      the situation, but it's a behavior change, personally I don't 
>>> think you want
>>>      to change the behavior, whether it is a "better" solution (not 
>>> just look better)
>>>      or "bug fix",  in this kind of clean-up project. Leave that to 
>>> separate "bug fix"
>>>      patch, if you believe the existing code is buggy/wrong.
>> Not changing this at all is not the intention of this. Even if the 
>> CloneNotSupportedException
>> cannot be thrown, chaining is what can be done to make it more clear 
>> what happend.
>> I don't think that i changed behavoir that much:
>>  - Returning null in Format will mostly (if not always) result in 
>> NullPointerException in
>>    the extending class.
>>  - DecimalFormat is the only class in jdk i found that makes it 
>> "correct" and catch it and throw
>>    an InternalError.
>>    If i think about how the catch in DecimalFormat maybe come into 
>> the codebase it must
>>    be that Format.clone must somehow returned null in the past. Which 
>> is a bizarre case,
>>    because it cannot happen cause Format is cloneable.
>>

We now started to "discuss" whether or not the original code is correct 
or not, and if not,
how to fix it. This is exactly the reason why I think this kind of 
"cleanup" project might not
want to get involved in, especially if it involves behavior change. Get 
that done separately
if you are interested to correct that wrong behavior. In this particular 
case, the "null return"
obviously "never" really happens, I agree it's fine to do so.

>>> Personally I think it's nice to make the switch to the new chaining 
>>> version if the
>>> original code is "new InternalError(msg) + initCause()", in which 
>>> case it has the
>>> clear intention that implementation does want to expose the original 
>>> cause of
>>> this InternalError. But for the case of InternalError(msg) only, the 
>>> proposed change
>>> actually again changes the behavior/intention of the original code, 
>>> in which it might
>>> not be desirable to expose the original cause when throw the 
>>> InternalError. As
>>> suggested in the Throwable API doc, the "hiding" might be purposely, 
>>> to separate
>>> different levels of abstraction when throw the InternalError.
>>
>> Two comments on this:
>> 1.    You maybe haven't noticed it. "new 
>> InternalError(msg).initCause()" is handled in a previous patch
>>        http://hg.openjdk.java.net/jdk8/tl/jdk/rev/c43af666d130
>>
>> 2.    You share this opinion with Xuelei.Fan. I think exactly the 
>> other way around. That
>>        maybe comes from my application development background where 
>> intensive logging
>>        and long stacktraces are the only information basis you got in 
>> production code.
>>        The only reason i see to be carefully with long 
>> exception-chains is GC and serialization/transfer-costs
>>        as i have written in
>>        
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2011-August/007562.html
>>        I will not say that Alan is supporting this meaning but he 
>> somewhat followed the idea in his follow-up.
Again, I'm not talking about which approach is correct, which one is 
wrong. I would leave
that to the owner of that particular area to decide, whether including 
the initial cause is
desired.
Generally I would assume if it is desired, then they can/should do so 
with initCause() at
first place (then, as I said, it's nice to make the switch to the new 
chaining version, in this
cleanup project). There are probably three possibilities why they did 
use initCause() at first
place (1) the original owner did not think the initial cause should be 
included (2) even
it should, since it doesn't matter, they did not include it (3) 
initCause() didn't even exist
when the original code was written.  Personally I think you can only do 
so if it's (2) or (3)
in a cleanup project. Given initCause() was added in 1.4, I would 
"assume/guess" most of
them are because of (3). and actually I believe most of them also belong 
to "doesn't really
matter" category.

That said, since 2d/awt, Xuelei.Fan have approved the changes in their 
areas, and Alan have
reviewed the changes as well (in which I would assume the core/lib area 
has been covered).
I will push the latest changes into the tl repository.

-Sherman