Kyle Galloway wrote:
Questions/Comments/Concerns?
I like it. Just some minor nits (some cosmetic, even).
Index: gnu/classpath/jdwp/value/Value.java
===================================================================
RCS file: gnu/classpath/jdwp/value/Value.java
diff -N gnu/classpath/jdwp/value/Value.java
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ gnu/classpath/jdwp/value/Value.java 2 Mar 2007 18:21:24 -0000
+ /**
+ * Calls the dervied classes writeValue method to write its value to the
+ * DataOutputStream.
+ *
+ * @param os write the value here
+ * @throws IOException
+ */
+ public void writeUntaggedValue(DataOutputStream os)
+ throws IOException
+ {
+ writeValue(os);
+ }
Can we shorten this to just "writeUntagged". It's just a nit: I hate to
type, and the ID system already uses this convention for un/tagged
object ids.
+ /**
+ * Will write the given object as a tagged value to the DataOutputStream.
+ *
+ * @param os write the value here
+ * @param obj the Object to write
+ * @throws IOException
+ */
+ public void writeTaggedValue(DataOutputStream os)
+ throws IOException
+ {
+ os.write (_tag);
+ writeValue(os);
+ }
Same here: "writeTagged".
+ protected abstract void writeValue(DataOutputStream os)
+ throws IOException;
Here I go again. Any need for the "Value" part of the name? It seems
redundant.
+ /**
+ * Creates a new Value of appropriate type for the value in the ByteBuffer
+ *
+ * @param bb contains the Object
+ * @param tag a byte tag indicating the type of the value in the ByteBuffer
+ * @return The Object referenced by the value
+ * @throws JdwpInternalErrorException
+ * @throws InvalidObjectException
+ */
+ public static Value createValueTagged(ByteBuffer bb, byte tag)
+ throws JdwpInternalErrorException, InvalidObjectException
I see that this method can throw InvalidObjectException if the object ID
is not known by the ID manager. How is JdwpInternalErrorException thrown?
Another little nit. The name of this method is: Value.createValueTagged.
Redundant and a little vague (to me at least). Can we rename this
Value.createFromTagged? [Likewise with the untagged variety.]
Hmm. This is really rather a factory method, isn't it? Perhaps we should
consider moving this method to a proper factory class. It's probably not
really worth it, but I could see it making the code a little
clearer/obvious. What do you think?
This method and the other static ones seem to all be declared something
like:
public static Object/Value METHODNAME (ByteBuffer bb, byte tag);
Yet every time one of these methods is called, it is always called like
so: "foo = METHODNAME(bb, bb.get());" I'd much rather see the methods
read from the ByteBuffer entirely, i.e., drop the "byte tag" part --
unless, of course, this is used somewhere else that I don't see yet.
> + case JdwpConstants.Tag.VOID:
> + val = new VoidValue();
> + case JdwpConstants.Tag.ARRAY:
> + case JdwpConstants.Tag.THREAD:
> + case JdwpConstants.Tag.OBJECT:
> + case JdwpConstants.Tag.THREAD_GROUP:
> + case JdwpConstants.Tag.CLASS_LOADER:
> + case JdwpConstants.Tag.CLASS_OBJECT:
> + ObjectId oid = VMIdManager.getDefault().readObjectId(bb);
> + val = new ObjectValue(oid.getObject());
> + case JdwpConstants.Tag.STRING:
> + val = new StringValue(JdwpString.readString(bb));
Missing a break for these cases. Did the compiler not output any
warnings? Please double-check.
+ public static Value createValueFromObject(Object value, Class klass)
This method is really, really similar to createValueTagged. Can you
rewrite so that this method uses getTagForClass and call
createValueTagged (or probably a common method)? [Obviously, you'll need
to tweak createValueTagged a little bit.]
+ public static Object getUntaggedObj(ByteBuffer bb, Class type)
+ throws JdwpInternalErrorException, InvalidObjectException
To parallel the tagged version of this method, please change the name to
"getUntaggedObject". As with the untagged version of this, I think I
might rather see these static methods go into a separate factory class.
Index: include/gnu_java_awt_peer_gtk_CairoSurface.h
===================================================================
RCS file:
/sources/classpath/classpath/include/gnu_java_awt_peer_gtk_CairoSurface.h,v
retrieving revision 1.9
diff -u -r1.9 gnu_java_awt_peer_gtk_CairoSurface.h
--- include/gnu_java_awt_peer_gtk_CairoSurface.h 21 Feb 2007 21:47:37
-0000 1.9
+++ include/gnu_java_awt_peer_gtk_CairoSurface.h 2 Mar 2007 18:21:26
-0000
:-)
Keith
