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
