Request for review: 7084245: Update usages of InternalError to use exception chaining

[Xueming Shen]

Hi Sebastian,

I will help to push the patch, if people all agreed the changes proposed.

I pulled your patch and generated the webrev at

http://cr.openjdk.java.net/~sherman/7084245/webrev

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.

(2) Removed your comment and left the printStackTrace() in LoopPipe.java
      untouched. Again, that  printStackTrace() might be unintentional, 
a leftover of
      the debug version for  example, but I'm not sure. Until the 2d 
engineer that is
      the case, I would prefer to leave it un-touched.

(3) Removed the concurrent package changes from the list. As discussed 
previously
      These changes might go different path to get it, if agreed.

It appears Xuelei.Fan from security might have some options on the 
changes made
in security area. So Xuelei, do you want me to pull out the changes in 
security
area and leave that for your to deal with separately?

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.

-Sherman




On 08/28/2011 12:35 PM, Sebastian Sickelmann wrote:
> Hi, here is a webrev[1] for some cleanup that i want to integrated in 
> tl-repositories.
>
> Alan Bateman had scanned the changes and gave me some good input[3] 
> for further discussion here:
>
> The changes to java.util.concurrent should go through Doug Lea's 
> upstream CVS. Alan told me that Chris Hegarty is on this topic 
> already. The suggested changes for this is here[2].
>
> I have changed some classes in awt / sun.java2d maybe someone of the 
> 2d-dev maillinglist can look at these changes.
> I also changed some classes in java/securtiy maybe someone of 
> security-dev maillinglist can look at these changes.
>
> Let me know if there is a need to split/rebase the main-webrev[1] to 
> review and/or push it individually.
>
> Mostly the patch changes exception-chains. But there are some places 
> where the patch changes behavoir:
>
> - I removed some printstackTraces in 
> sun.java2d.pipe.LoopPipe.getStrokesSpans and sun.misc.Launcher (Alan 
> told me that kumar maybe want to have a look at it?).
> - I changed java.text.Format.clone not to return null. I think it will 
> never happen. But if so throwing an InternalError seems to be better 
> than returning null and let all the extended classes crash in there 
> clone Method with a NullPointerException. And so catching an Exception 
> in java.text.DecimalFormat.clone is unnecessary.
>
> -- Sebastian
>
> [1] http://oss-patches.24.eu/openjdk8/InternalError/part2/7084245_main_0/
> [2] 
> http://oss-patches.24.eu/openjdk8/InternalError/part2/7084245_concurrent_0/
> [3] 
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2011-August/007563.html