[OPTIMIZED] java.lang

[Godmar Back]

 Robert,

I don't have a whole lot of time right now, but a couple of comments
about your patches.  First off, we do appreciate them.

However, formatting changes make them awfully hard to evaluate.
Would it be possible for you to submit a version that would only contain
substantial changes, i.e., no formatting changes?

Let me go over your list and tell you what I would like to see:

> 
> * Runtime.runFinalizersOnExit(boolean) must be static

Good.

> * Serializable declarations and serialVersionUID definitions completed

The declarations are fine, but it's not at all clear whether we need
the serialVersionUIDs.  See FAQ.Known-bugs.  Tim recently fixed 
serialization, we need to research whether we still need the serialVersionUIDs 
or not and if so, where.
I'm not saying we don't need them, I'm just saying it's unclear to me 
at this point.

> * Integer.decode(String) threw NumberFormatException on pure "0" (zero)
> * some uninstantiable classes missed the private constructor

Good.

> * do not observe String internals from classes other than StringBuffer

Maybe, but probably good.

> * wrong conversion value in Character.toTitleCase(char)
> * some StringIndexOutOfBoundsException checks were missing
> * some checkPropertiesAccess()s were missing from 

Good.

> * the rest is heavy code optimization and clean-up

I looked at the changes some, and I do not consider replacing if/else 
with the ? operator an optimization.  In fact, the authors of the code
made a conscious choice to avoid the ? operator.  This is in part because
? is buggy in pizza, but more importantly, it's because the ? operator
is hard to read and error-prone in general.

This is not to say that part of kaffe's coding style is to not use the
? operator, which would be kind of silly since I said Kaffe doesn't have
a coding style, but please only use it in your own, new code.

> 
> one thing I was not really sure how to deal with:
> 
> take an as-of-the-spec-synchronized method which itself does not do 
> anything dangerous, but calls an other synchronized method e.g.:
> 
> public final /* synchronized */ void join(long millis) throws InterruptedException
> {
>         join(millis, 0);
> }
> 
> do we need this method to be explicitly synchronized conforming to the spec, 
> or we can omit the synchronized keyword as that is an implementation 
> detail and our code is "more optimized" without that?
> 
> I was brave and commented out 9 occurences of the synchronized keyword,
> tested and it seems to be OK. you can easily withdraw these changes in
> the patch: search for the '/\* synchronized \*/' regexp.
> 

I need to read up on this, but I vaguely remember somebody saying that
synchronized is no longer part of the spec.  That is, if a function
is not thread-safe, it must protect itself somehow, but that's not part of the
interface.  Should that turn out to be true (somebody check this), then
yes, let's omit synchronized --- but only where it's immediately obvious that 
it's not needed.

	- Godmar