Re: [OpenJDK 2D-Dev] Request for review: 7084245: Update usages of InternalError to use exception chaining
Am 28.08.2011 22:54, schrieb Mario Torre:
Il giorno dom, 28/08/2011 alle 21.35 +0200, Sebastian Sickelmann ha
scritto:
Hi, here is a webrev[1] for some cleanup that i want to integrated in
tl-repositories.
Hi,
Hi created a new webrev with your suggested changes at:
http://oss-patches.24.eu/openjdk8/InternalError/part2/7084245_main_1/
I have created some comments inline in this mail.
The patch seems good for me (speaking in particular about the AWT/2D
part), and I don't think it will introduce any side effects.
Just a note:
src/share/classes/java/lang/invoke/CallSite.java:
} catch (ReflectiveOperationException ignore) {
-throw new InternalError();
+throw new InternalError(ignore);
}
Being a refactoring patch, I would not change the name from ignore to
the more classical e, since we are not ignoring the exception anymore.
Also in src/share/classes/com/sun/servicetag/BrowserSupport.java, it
seems to me that you can also aggregate the InvocationTargetException in
the multi catch.
I think we should not make further aggregating on InvocationTargetException
in this patch. It changes behavoir. Which of the two places to you mean
we can
aggregate into multi catch?
In fact, the block could be probably rewritten to take advantage of
ReflectiveOperationException.
Same for src/share/classes/sun/misc/Launcher.java,
src/share/classes/sun/security/util/SecurityConstants.java and
src/share/classes/sun/tracing/dtrace/DTraceProvider.java
Changed that to ReflectiveOperationException.
Cheers,
Mario
-- Sebastian
Re: Request for review: 7084245: Update usages of InternalError to use exception chaining
Am 29.08.2011 21:10, schrieb Xueming Shen: Hi Sebastian, I will help to push the patch, if people all agreed the changes proposed. Thanks for supporting this. I pulled your patch and generated the webrev at http://cr.openjdk.java.net/~sherman/7084245/webrev I already created a new webrev that handles the suggested changes of Mario Torre. They sounded reasonable for me. 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. (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. Done a special webrev for this here: http://oss-patches.24.eu/openjdk8/InternalError/part2/7084245_concurrent_0/ 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? Xuelei.Fan gave some input for the security area. He said quote Exceptions thrown by a method should be defined at an abstraction level consistent with what the method does, not necessarily with the low-level details of how it is implemented. quote/ and i think he is totally right. But if i read it right in Throwable this is exactly what the Throwable.cause is intended for. quote * One reason that a throwable may have a cause is that the class that * throws it is built atop a lower layered abstraction, and an operation on * the upper layer fails due to a failure in the lower layer. It would be bad * design to let the throwable thrown by the lower layer propagate outward, as * it is generally unrelated to the abstraction provided by the upper layer. * Further, doing so would tie the API of the upper layer to the details of * its implementation, assuming the lower layer's exception was a checked * exception. Throwing a wrapped exception (i.e., an exception containing a * cause) allows the upper layer to communicate the details of the failure to * its caller without incurring either of these shortcomings. It preserves * the flexibility to change the implementation of the upper layer without * changing its API (in particular, the set of exceptions thrown by its * methods). quote/ 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
Re: Request for review: 7084245: Update usages of InternalError to use exception chaining
Sorry i have forgotten the webrev url. http://oss-patches.24.eu/openjdk8/InternalError/part2/7084245_main_1/ Am 30.08.2011 10:20, schrieb Sebastian Sickelmann: Am 29.08.2011 21:10, schrieb Xueming Shen: Hi Sebastian, I will help to push the patch, if people all agreed the changes proposed. Thanks for supporting this. I pulled your patch and generated the webrev at http://cr.openjdk.java.net/~sherman/7084245/webrev I already created a new webrev that handles the suggested changes of Mario Torre. They sounded reasonable for me. 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. (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. Done a special webrev for this here: http://oss-patches.24.eu/openjdk8/InternalError/part2/7084245_concurrent_0/ 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? Xuelei.Fan gave some input for the security area. He said quote Exceptions thrown by a method should be defined at an abstraction level consistent with what the method does, not necessarily with the low-level details of how it is implemented. quote/ and i think he is totally right. But if i read it right in Throwable this is exactly what the Throwable.cause is intended for. quote * One reason that a throwable may have a cause is that the class that * throws it is built atop a lower layered abstraction, and an operation on * the upper layer fails due to a failure in the lower layer. It would be bad * design to let the throwable thrown by the lower layer propagate outward, as * it is generally unrelated to the abstraction provided by the upper layer. * Further, doing so would tie the API of the upper layer to the details of * its implementation, assuming the lower layer's exception was a checked * exception. Throwing a wrapped exception (i.e., an exception containing a * cause) allows the upper layer to communicate the details of the failure to * its caller without incurring either of these shortcomings. It preserves * the flexibility to change the implementation of the upper layer without * changing its API (in particular, the set of exceptions thrown by its * methods). quote/ 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.
Re: Request for review: 7084245: Update usages of InternalError to use exception chaining
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
Re: Request for review: 7084245: Update usages of InternalError to use exception chaining
Sebastian, src/share/classes/sun/misc/Launcher.java in new patch appears to miss a ending } at ln#487 -Sherman On 08/30/2011 10:20 AM, Xueming Shen wrote: 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
Re: Request for review: 7084245: Update usages of InternalError to use exception chaining
Am 29.08.2011 02:02, schrieb David Holmes: On 29/08/2011 5:35 AM, 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]. These changes are somewhat pointless. We don't need to set the cause because it will never happen. The classes are cloneable so CloneNotSupportedException can not be thrown. If it is then something really bizarre has happened hence we throw InternalError. The site of the InternalError tells us why it was thrown. David - Thanks for your input. I have myself thought many times about the effects of these changes. It changes almost nothing, especially because it chains to InternalError which should not be thrown in the most cases. So chaining almost costs us zero runtime-costs but in really bizarre situations it gives slightly more information what happend. This cleanup was the first step in a series of cleanup work i want to do in topic of exception-chaining. I chose it(InternalError) as the first step because it it has so less side-effect(longer chains, runtime performance while serializing and printing, later GC of causes,...) because it should mostly never happen. So i had the change to learn using the tools(mercurial, openjdk build files, webrev, contribution process). The only problem i actually see with these small-changes (where nothing really bad/wrong is cleanuped) is the workload i generate for the reviewers, supporters, mailinglist-readers and if its worth doing it anyway. So let me know if the complete cleanup of exception chaining should be considered to stop. It's ok if it's so. I have learned enought in using the tools and i can start searching for more interessting things to do. I only keep going because i said i will do it. It the point in time (Aug 8) i guessed that there is much to do if I start with it. But i haven't imagine that's so much assiduity work. -- Sebastian
Re: Request for review: 7084245: Update usages of InternalError to use exception chaining
I reviewed the security part. In general, it looks fine. But sometimes,
from my very personal view, the exception-chain might be over used. For
example, the following code:
try {
md = MessageDigest.getInstance(SHA);
} catch (NoSuchAlgorithmException nsae) {
throw new InternalError(internal error: SHA-1 not available.);
}
The exception message and stack is simple and easy to understand. If
using exception-chain, the message chain looks like:
internal error: SHA-1 not available.
- SHA ... not available
I think the information is redundant, and expose unnecessary call stack.
Exceptions thrown by a method should be defined at an abstraction level
consistent with what the method does, not necessarily with the low-level
details of how it is implemented. Please also refer to item 61, Throw
exceptions appropriate to the abstraction, in Effective Java 2nd Editor.
It seems that the webrev offline now, I cannot access it.
Thanks,
Xuelei
On 8/29/2011 3:35 AM, 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
Re: Request for review: 7084245: Update usages of InternalError to use exception chaining
Hi, Sorry for the delay, was on vacation...comment inlined. 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?). The methods in question in sun.misc.Launcher are not called by the java launcher, I think these were added to facilitate the JavaWebstart Launcher javaws. Nevertheless, the changes look good to me. Thanks Kumar - 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
Re: Request for review: 7084245: Update usages of InternalError to use exception chaining
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
Re: Request for review: 7084245: Update usages of InternalError to use exception chaining
On 29/08/2011 5:35 AM, 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]. These changes are somewhat pointless. We don't need to set the cause because it will never happen. The classes are cloneable so CloneNotSupportedException can not be thrown. If it is then something really bizarre has happened hence we throw InternalError. The site of the InternalError tells us why it was thrown. David - 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
