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
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.
I think we should think about it and get some more general policy when
to chain and when not and document it
in Throwable javadoc more clearly. I cannot find the place in Throwable
API you mentioned about hiding.
-Sherman
-- Sebastian
