Re: vm/reference reflection update

[Mark Wielaard]

Hi Grzegorz,

On Sun, 2004-03-28 at 07:22, Grzegorz B. Prokopski wrote:
> This patch adds to the reference implementation this reflection
> functionality, which can be realized in Java. 

You should not commit such large non-obvious patches before discussing
them on-list first. I know on irc I said that I thought you should just
push as much as you have now in sablevm-classpath into classpath. But I
didn't mean you should commit it without discussion. I like the fact
that you do most things here in java since that is easy to reusable by
most VMs. But large changes like this should be discussed first
publicly.

Please revert this patch for now and then lets discuss how/why to put
which things where.

> 2004-03-28  Grzegorz B. Prokopski <address@hidden> 
> 
>         * Merged vm/reference implementation of these reflection elements,
>         that can be realized in Java:
>         vm/reference/java/lang/reflect/Constructor.java,
>         vm/reference/java/lang/reflect/Field.java,
>         vm/reference/java/lang/reflect/Makefile.am,
>         vm/reference/java/lang/reflect/Method.java,
>         vm/reference/java/lang/reflect/ReflectUtil.java.

This isn't a proper ChangeLog message.
See http://mail.gnu.org/archive/html/classpath/2003-08/msg00066.html
(I'll put that on the list to add that to the Hacker Guide.)

I don't have time to go through the whole diff now (it is 24 pages...)
but here are some general comments about it.

- Don't keep commented out code. Just remove it or add a real comment
  describing what it used to do and why it is changed to the current
  implementation.

- You should follow (the unwritten, I know) coding style guide.
  http://mail.gnu.org/archive/html/classpath/2003-10/msg00121.html
  We kind of have a tool for that, but that still isn't really
  available/released. Tom, what is the latest status?
  See also http://gcc.gnu.org/ml/java/2003-12/msg00110.html

- Classes that are not vm specific like the ReflectUtil class should go
  into java/util/reflect, not under vm/reference.

- When changing the reference classes you should add an entry to the
  NEWS file and the VM Integration guide.

- Now that you have most of this in java source it should be split into
  common java/lang/reflect and vm/reference/java/lang/reflect VM classes
  that contain only the essential (native) plug-points for the VM.
  (And if possible a standard JNI/Posix reference implementation.)

Could you look into the above issues and resubmit a patch?
Or give comments about the above points?

Thanks,

Mark

signature.asc
Description: This is a digitally signed message part