Re: The right way(tm) of writing toString() (Was: Re: [PATCH] Field position attribute handling)
Dalibor Topic wrote: Hi Tom, Tom Tromey wrote: Dalibor == Dalibor Topic [EMAIL PROTECTED] writes: Dalibor here's my attempt at writing a short passage on the Art of Writing Dalibor toString methods. As everyone has written bazillions of them, it's a Dalibor perfect subject for bikeshed type of discussions. Feel free to chip in Dalibor with your own bits of wisdom ;) My bit of wisdom is: please put this in the classpath hacker's guide. We really should be collecting these how-tos as part of our coding standard, for all the usual reasons... Thanks for your comment. I'll check it in tonight, with the chenges proposed by you and Victor. Remember that the example that i used (with HttpURLConnection) was wrong. ___ Classpath mailing list [EMAIL PROTECTED] http://mail.gnu.org/mailman/listinfo/classpath
Re: The right way(tm) of writing toString() (Was: Re: [PATCH] Field position attribute handling)
Dalibor == Dalibor Topic [EMAIL PROTECTED] writes: Dalibor here's my attempt at writing a short passage on the Art of Writing Dalibor toString methods. As everyone has written bazillions of them, it's a Dalibor perfect subject for bikeshed type of discussions. Feel free to chip in Dalibor with your own bits of wisdom ;) My bit of wisdom is: please put this in the classpath hacker's guide. We really should be collecting these how-tos as part of our coding standard, for all the usual reasons... Tom ___ Classpath mailing list [EMAIL PROTECTED] http://mail.gnu.org/mailman/listinfo/classpath
Re: The right way(tm) of writing toString() (Was: Re: [PATCH] Field position attribute handling)
Hi Tom, Tom Tromey wrote: Dalibor == Dalibor Topic [EMAIL PROTECTED] writes: Dalibor here's my attempt at writing a short passage on the Art of Writing Dalibor toString methods. As everyone has written bazillions of them, it's a Dalibor perfect subject for bikeshed type of discussions. Feel free to chip in Dalibor with your own bits of wisdom ;) My bit of wisdom is: please put this in the classpath hacker's guide. We really should be collecting these how-tos as part of our coding standard, for all the usual reasons... Thanks for your comment. I'll check it in tonight, with the chenges proposed by you and Victor. cheers, dalibor topic ___ Classpath mailing list [EMAIL PROTECTED] http://mail.gnu.org/mailman/listinfo/classpath
Re: 3d attempt at Re: The right way(tm) of writing toString() (Was: Re: [PATCH] Field position attribute handling)
Sorry i meant the HttpURLConnection Class not URL Class ___ Classpath mailing list [EMAIL PROTECTED] http://mail.gnu.org/mailman/listinfo/classpath
Re: 3d attempt at Re: The right way(tm) of writing toString() (Was: Re: [PATCH] Field position attribute handling)
Hi all
Salut Etienne,
Etienne Gagnon wrote:
Hi Dalibor,
I would drop h) altogether.
O.K.,o.k., so be it.
Rationale: while a naive Java compiler would produce a series of iload
instructions, for reusing a local variable, an optimizing compiler or
a jit
would completely remove this. So, yes, the code that reuse a local
could be
a little slower on a interpreter (I am sure that on sablevm this
would be
negligible as iload is a very cheap operation), but good software
engineering
is the goal here.
Here's the latest version:
a) toString output should begin with the name of the class
Rationale: When a field is declared as having an interface type, or
a non-final class, it is useful to know the exact type of the instance.
I have noticed this is not true on Sun's SDK for example URL class
toString() returns http://host/path/file...; if i try the same
with the classpath URL class and i get something like:
gnu.java.net.protocol.http.Connection:http://directory.google.com/Top/;
b) the rest of the string should be braced in '[' and ']'
Rationale: Visual separation makes it easier to separate the output
string into its type information and information about the state of
the particular instance.
c) different instances should have different string representations
Rationale: If two instances are not equal, then their string
represenation should reflect that. The contents of all fields used in
equals() should be part of the string representation of an instance.
d) fields that are used in the string representation must be named
Rationale: Explicitely naming fields removes ambiguities when a
class contains several fields of the same type.
e) always use a StringBuffer
Rationale: While most Java compilers can optimize string
concatenation by using string buffers automatically, that only works
efficiently as long as the string representation can be generated
within a single expression. If that's not the case, for example when
iterating over a collection, most Java compilers create unnecessary
temporary objects. Using a StringBuffer ensures that toString() works
efficiently.
f) don't use toString() on non-primitive fields
Rationale: Calling toString() can fail if a field's instance in
null, resulting in a NullPointerException being thrown inside
toString(). Since toString() is usually used to provide debugging
information, it must not fail. Using StringBuffer.append(Object)
handles null automatically. So simply use StringBuffer's append method
for field_name= and the field instance.
g) don't write special code to handle fields being null
Rationale: follows from f).
Example code:
public class Test{
private String field_1;
private int field_2;
public String toString() {
StringBuffer sb = new StringBuffer(Test[field_1=));
sb.append(field_1);
sb.append(,field_2=);
sb.append(field_2);
sb.append(']');
return sb.toString();
}
}
comments are welcome, as usual.
cheers,
dalibor topic
___
Classpath mailing list
[EMAIL PROTECTED]
http://mail.gnu.org/mailman/listinfo/classpath
___
Classpath mailing list
[EMAIL PROTECTED]
http://mail.gnu.org/mailman/listinfo/classpath
Re: 3d attempt at Re: The right way(tm) of writing toString() (Was: Re: [PATCH] Field position attribute handling)
I was mistaken sorry about my big mouth ___ Classpath mailing list [EMAIL PROTECTED] http://mail.gnu.org/mailman/listinfo/classpath
Re: 3d attempt at Re: The right way(tm) of writing toString() (Was: Re: [PATCH] Field position attribute handling)
Victor == Victor Niebla [EMAIL PROTECTED] writes: Victor I have noticed this is not true on Sun's SDK for example URL class Victor toString() returns http://host/path/file...; if i try the same Victor with the classpath URL class and i get something like: Victor gnu.java.net.protocol.http.Connection:http://directory.google.com/Top/; Yeah, there are some cases where the toString() result is clearly more useful without class information. I think Dalibor's rules should only apply in cases where toString either (a) isn't specified by Sun or (b) doesn't have a clear programmatic use. Tom ___ Classpath mailing list [EMAIL PROTECTED] http://mail.gnu.org/mailman/listinfo/classpath
2nd attempt at Re: The right way(tm) of writing toString() (Was: Re: [PATCH] Field position attribute handling)
Salut Etienne,
Etienne Gagnon wrote:
Dalibor Topic wrote:
e) use '+' to concatenate strings and objects
Rationale: Most Java compilers can optimize string concatenation by
using string buffers. There is no need to do that task by hand. Using
'+' allows you to write simpler code.
I partly disagree. When you iterate through a collection, using + will
create one stringbuffer per iteration = bad.
So, I would in fact recommend using stringbuffers everywhere, unless the
toString() body holds on a single source line.
Thanks for pointing that out, I had forgot that.
f) don't use toString() on non-primitive fields. Use field_name= +
field instead.
or
sb.append(field_name=);
sb.append(field);
yep.
g) don't write special code to handle fields being null
I think it also works for sb.append().
yes, it does.
Example code:
public class Test{
private String field_1;
private int field_2;
public String toString() {
return (Test
+ [field_1= + field_1
+ , field_2= + field_2
+ ']');
This is a logical single line, so it fits the non-StringBuffer thing.
But,
this does not work for LinkedList.toString(), for example.
Etienne
p.s. if performace of toString() doesn't matter that much, and we can
use reflection
No, please! We want toString to be fast. It is part of the base
functionality
of Object(), so there is no reason to assume programmers will only use
it for
debugging.
O.K., then I'll attempt to write down how to make toString() fast with
StringBuffers.
so here's the updated version:
a) toString output should begin with the name of the class
Rationale: When a field is declared as having an interface type, or a
non-final class, it is useful to know the exact type of the instance.
b) the rest of the string should be braced in '[' and ']'
Rationale: Visual separation makes it easier to separate the output
string into its type information and information about the state of the
particular instance.
c) different instances should have different string representations
Rationale: If two instances are not equal, then their string
represenation should reflect that. The contents of all fields used in
equals() should be part of the string representation of an instance.
d) fields that are used in the string representation must be named
Rationale: Explicitely naming fields removes ambiguities when a class
contains several fields of the same type.
e) always use a StringBuffer
Rationale: While most Java compilers can optimize string
concatenation by using string buffers automatically, that only works
efficiently as long as the string representation can be generated within
a single expression. If that's not the case, for example when iterating
over a collection, most Java compilers create unnecessary temporary
objects. Using a StringBuffer ensures that toString() works efficiently.
f) don't use toString() on non-primitive fields
Rationale: Calling toString() can fail if a field's instance in null,
resulting in a NullPointerException being thrown inside toString().
Since toString() is usually used to provide debugging information, it
must not fail. Using StringBuffer.append(Object) handles null
automatically. So simply use StringBuffer's append method for
field_name= and the field instance.
g) don't write special code to handle fields being null
Rationale: follows from f).
h) re-use the StringBuffer on the stack
Rationale: In order to write as efficient code using StringBuffer as
the good Java compilers, we need to reuse the StringBuffer instance on
the stack. So instead of writing multiple append statements, try to use
a single statement to perform all the work on the StringBuffer instance,
including creation, calling append, and calling toString() on it.
Example code:
public class Test{
private String field_1;
private int field_2;
public String toString() {
return
(new StringBuffer(Test[field_1=)).append(field_1)
.append(,field_2=).append(field_2)
.append(']')
.toString();
}
}
comments are welcome, as usual.
cheers,
dalibor topic
___
Classpath mailing list
[EMAIL PROTECTED]
http://mail.gnu.org/mailman/listinfo/classpath
Re: 2nd attempt at Re: The right way(tm) of writing toString() (Was: Re: [PATCH] Field position attribute handling)
Quoting Dalibor Topic ([EMAIL PROTECTED]): h) re-use the StringBuffer on the stack Rationale: In order to write as efficient code using StringBuffer as the good Java compilers, we need to reuse the StringBuffer instance on the stack. So instead of writing multiple append statements, try to use a single statement to perform all the work on the StringBuffer instance, including creation, calling append, and calling toString() on it. That would only help with interpreters, right? Nothing a good JIT does not handle for you. pgp0.pgp Description: PGP signature ___ Classpath mailing list [EMAIL PROTECTED] http://mail.gnu.org/mailman/listinfo/classpath
Re: 2nd attempt at Re: The right way(tm) of writing toString() (Was: Re: [PATCH] Field position attribute handling)
Hi David, David Lichteblau wrote: Quoting Dalibor Topic ([EMAIL PROTECTED]): h) re-use the StringBuffer on the stack Rationale: In order to write as efficient code using StringBuffer as the good Java compilers, we need to reuse the StringBuffer instance on the stack. So instead of writing multiple append statements, try to use a single statement to perform all the work on the StringBuffer instance, including creation, calling append, and calling toString() on it. That would only help with interpreters, right? Nothing a good JIT does not handle for you. I doubt that it only helps with interpreters. I'd assume that it also helps with kaffe's JIT, for example. But every little bit helps ;) Basically, you want to have a JIT that rewrites sb.append(something); sb.append(else); into sb.append(something).append(else); In order to do that, the JIT would have to infer that StringBuffer.append(*) *always* returns a reference to the same string buffer instance it was invoked upon in that code path. as in : [sb on stack] [call append] [hey, that's sb on stack again, cool!] Which means you either hardcode the optimisation into the JIT (which is possible since StringBuffer is a final class), or do some bytecode analysis on side-effects of StringBuffer.append() (i.e. does it return 'this' on all code paths). I don't know how hard the second approach is. In any case, the question is: can we assume that VMs using GNU Classpath have good JITs? I think the answer is 'No', despite the great performance of some VMs using GNU Classpath. Some VMs using GNU Classpath have no JITs, some have JITs that aren't state-of-the-art. On a side note, I would have loved to assume that VMs using GNU Classpath have fast reflection implementations, which would have allowed me to use the generic, reflection based method outlined in the first proposal. But being a project that's used by multiple VMs, I think GNU Classpath has to care about the lowest common denominator. Which means I can't assume that some bytecode optimizer will take care of performance of my code ;) cheers, dalibor topic ___ Classpath mailing list [EMAIL PROTECTED] http://mail.gnu.org/mailman/listinfo/classpath
Re: 2nd attempt at Re: The right way(tm) of writing toString() (Was: Re: [PATCH] Field position attribute handling)
Quoting Dalibor Topic ([EMAIL PROTECTED]): I doubt that it only helps with interpreters. I'd assume that it also helps with kaffe's JIT, for example. But every little bit helps ;) Fair enough. I will have to learn about kaffe's JIT then. Basically, you want to have a JIT that rewrites sb.append(something); sb.append(else); into sb.append(something).append(else); More precisely, a JIT that compiles both forms into machine code of similar performance, where the difference is probably a load of `sb' from memory for each method invocation. (A sufficiently smart compiler might do what you are describing, but that sounds a little unrealistic to me, too.) In any case, the question is: can we assume that VMs using GNU Classpath have good JITs? I think the answer is 'No', despite the great Well, I guess that was my question. Thanks, David pgp0.pgp Description: PGP signature ___ Classpath mailing list [EMAIL PROTECTED] http://mail.gnu.org/mailman/listinfo/classpath
Re: 2nd attempt at Re: The right way(tm) of writing toString() (Was: Re: [PATCH] Field position attribute handling)
Hi Dalibor,
I would drop h) altogether.
Rationale: while a naive Java compiler would produce a series of iload
instructions, for reusing a local variable, an optimizing compiler or a jit
would completely remove this. So, yes, the code that reuse a local could be
a little slower on a interpreter (I am sure that on sablevm this would be
negligible as iload is a very cheap operation), but good software engineering
is the goal here.
Not only do I personally find the following more readable, but it is also
easier to debug, as a debugger would trace through the instructions one by one:
public String toString()
{
StringBuffer sb = StringBuffer();
sb.append(Test[field_1=);
sb.append(field_1);
sb.append(,field_2=);
sb.append(field_2);
sb.append(]);
return sb.toString();
}
The performance difference (except on naive interpreters) is negligible.
Note about performance: The reason iload is expensive on naive interpreters,
is the cost of dispatching an additional instruction in the interpreter.
SableVM's inline-threaded engine does eliminate dispatch between successive
instructions of a basic block, so the cost of an additional iload is simply
that of the iload functionality, not that of (iload + dispatch) as found
in normal switch-based interpreters and direct-threaded interpreters.
So, in summary: it is best to leave such low-level optimizations in the hand
of the compiler, not of the developer. The developer should instead care about
code readability and debuggability (i.e. software engineering issues).
Etienne
Dalibor Topic wrote:
h) re-use the StringBuffer on the stack
Rationale: In order to write as efficient code using StringBuffer as
the good Java compilers, we need to reuse the StringBuffer instance on
the stack. So instead of writing multiple append statements, try to use
a single statement to perform all the work on the StringBuffer instance,
including creation, calling append, and calling toString() on it.
Example code:
public class Test{
private String field_1;
private int field_2;
public String toString() {
return
(new StringBuffer(Test[field_1=)).append(field_1)
.append(,field_2=).append(field_2)
.append(']')
.toString();
}
}
comments are welcome, as usual.
cheers,
dalibor topic
--
Etienne M. Gagnon, Ph.D. http://www.info.uqam.ca/~egagnon/
SableVM: http://www.sablevm.org/
SableCC: http://www.sablecc.org/
___
Classpath mailing list
[EMAIL PROTECTED]
http://mail.gnu.org/mailman/listinfo/classpath
3d attempt at Re: The right way(tm) of writing toString() (Was: Re: [PATCH] Field position attribute handling)
Salut Etienne,
Etienne Gagnon wrote:
Hi Dalibor,
I would drop h) altogether.
O.K.,o.k., so be it. ;)
Rationale: while a naive Java compiler would produce a series of iload
instructions, for reusing a local variable, an optimizing compiler or a jit
would completely remove this. So, yes, the code that reuse a local
could be
a little slower on a interpreter (I am sure that on sablevm this would be
negligible as iload is a very cheap operation), but good software
engineering
is the goal here.
Here's the latest version:
a) toString output should begin with the name of the class
Rationale: When a field is declared as having an interface type, or a
non-final class, it is useful to know the exact type of the instance.
b) the rest of the string should be braced in '[' and ']'
Rationale: Visual separation makes it easier to separate the output
string into its type information and information about the state of the
particular instance.
c) different instances should have different string representations
Rationale: If two instances are not equal, then their string
represenation should reflect that. The contents of all fields used in
equals() should be part of the string representation of an instance.
d) fields that are used in the string representation must be named
Rationale: Explicitely naming fields removes ambiguities when a class
contains several fields of the same type.
e) always use a StringBuffer
Rationale: While most Java compilers can optimize string
concatenation by using string buffers automatically, that only works
efficiently as long as the string representation can be generated within
a single expression. If that's not the case, for example when iterating
over a collection, most Java compilers create unnecessary temporary
objects. Using a StringBuffer ensures that toString() works efficiently.
f) don't use toString() on non-primitive fields
Rationale: Calling toString() can fail if a field's instance in null,
resulting in a NullPointerException being thrown inside toString().
Since toString() is usually used to provide debugging information, it
must not fail. Using StringBuffer.append(Object) handles null
automatically. So simply use StringBuffer's append method for
field_name= and the field instance.
g) don't write special code to handle fields being null
Rationale: follows from f).
Example code:
public class Test{
private String field_1;
private int field_2;
public String toString() {
StringBuffer sb = new StringBuffer(Test[field_1=));
sb.append(field_1);
sb.append(,field_2=);
sb.append(field_2);
sb.append(']');
return sb.toString();
}
}
comments are welcome, as usual.
cheers,
dalibor topic
___
Classpath mailing list
[EMAIL PROTECTED]
http://mail.gnu.org/mailman/listinfo/classpath
The right way(tm) of writing toString() (Was: Re: [PATCH] Field position attribute handling)
Hi Mark,
Mark Wielaard wrote:
Hi,
On Sun, 2003-11-23 at 21:40, Dalibor Topic wrote:
2003-11-23 Dalibor Topic [EMAIL PROTECTED]
* java/text/FieldPosition.java (equals): Fixed comment.
Looks fine. Please check it in.
It would be nice if we had something about the right way (tm) of writing
the standard clone(), toString(), hashCode() and equals() in the Hackers
Guide. (hint...)
I see. I'll try to come up with something based on what I've seen on the
web and Bloch's book.
here's my attempt at writing a short passage on the Art of Writing
toString methods. As everyone has written bazillions of them, it's a
perfect subject for bikeshed type of discussions. Feel free to chip in
with your own bits of wisdom ;)
a) toString output should begin with the name of the class
Rationale: When a field is declared as having an interface type, or a
non-final class, it is useful to know the exact type of the instance.
b) the rest of the string should be braced in '[' and ']'
Rationale: Visual separation makes it easier to separate the output
string into its type information and information about the state of the
particular instance.
c) different instances should have different string representations
Rationale: If two instances are not equal, then their string
represenation should reflect that. The contents of all fields used in
equals() should be part of the string representation of an instance.
d) fields that are used in the string representation must be named
Rationale: Explicitely naming fields removes ambiguities when a class
contains several fields of the same type.
e) use '+' to concatenate strings and objects
Rationale: Most Java compilers can optimize string concatenation by
using string buffers. There is no need to do that task by hand. Using
'+' allows you to write simpler code.
f) don't use toString() on non-primitive fields. Use field_name= +
field instead.
Rationale: Calling toString() can fail if a field's instance in null,
resulting in a NullPointerException being thrown inside toString().
Since toString() is usually used to provide debugging information, it
must not fail. Using '+' as explained above lets the compiler do the job
of calling StringBuffer.append(Object) or String.valueOf(Object), that
both can handle null automativally.
g) don't write special code to handle fields being null
Rationale: follows from f).
Example code:
public class Test{
private String field_1;
private int field_2;
public String toString() {
return (Test
+ [field_1= + field_1
+ , field_2= + field_2
+ ']');
}
}
comments are welcome, as usual.
cheers,
dalibor topic
p.s. if performace of toString() doesn't matter that much, and we can
use reflection, it would be quite simple to write a
gnu/classpath/ToString class with a stringize method that could
automatically create string representations conforming to above rules,
given the interesting fields of a class.
it basically comes down to:
stringize(Field [] fields)
getClass().getName()
+ '['
+ ( for all f in fields : f.getName().toLower() + '=' + f.get() +
separator(, ), if necessary)
Is there some interest in that?
___
Classpath mailing list
[EMAIL PROTECTED]
http://mail.gnu.org/mailman/listinfo/classpath
Re: The right way(tm) of writing toString() (Was: Re: [PATCH] Field position attribute handling)
Dalibor Topic wrote:
e) use '+' to concatenate strings and objects
Rationale: Most Java compilers can optimize string concatenation by
using string buffers. There is no need to do that task by hand. Using
'+' allows you to write simpler code.
I partly disagree. When you iterate through a collection, using + will
create one stringbuffer per iteration = bad.
So, I would in fact recommend using stringbuffers everywhere, unless the
toString() body holds on a single source line.
f) don't use toString() on non-primitive fields. Use field_name= +
field instead.
or
sb.append(field_name=);
sb.append(field);
g) don't write special code to handle fields being null
I think it also works for sb.append().
Example code:
public class Test{
private String field_1;
private int field_2;
public String toString() {
return (Test
+ [field_1= + field_1
+ , field_2= + field_2
+ ']');
This is a logical single line, so it fits the non-StringBuffer thing. But,
this does not work for LinkedList.toString(), for example.
Etienne
p.s. if performace of toString() doesn't matter that much, and we can
use reflection
No, please! We want toString to be fast. It is part of the base functionality
of Object(), so there is no reason to assume programmers will only use it for
debugging.
Etienne
--
Etienne M. Gagnon, Ph.D. http://www.info.uqam.ca/~egagnon/
SableVM: http://www.sablevm.org/
SableCC: http://www.sablecc.org/
___
Classpath mailing list
[EMAIL PROTECTED]
http://mail.gnu.org/mailman/listinfo/classpath
Re: [PATCH] Field position attribute handling
Mark Wielaard wrote: Hi, On Wed, 2003-11-19 at 02:14, Dalibor Topic wrote: The documentation and code don't match up now. My Class Library book seems to indicate that it actually is instanceof, thought in most cases your code comparing the actual Class seems more correct. It's the recommended way of doing equals() if you want to avoid symmetry bugs. See PRAXIS 12 in Peter Haggar's Practical Java, or §5.1, p 119 in Java 2 performance and idion guide by craig lahrmann rhett guthrie, or Effective Java by Joshua Bloch. Using instanceof is a bad idea because it allows for symmetry bugs: I know. But that wasn't the point. If you change to code then you must also update the documentation. O.K., I just wans;t very sure about a non-ambigous wording. How about the attached patch? 2003-11-23 Dalibor Topic [EMAIL PROTECTED] * java/text/FieldPosition.java (equals): Fixed comment. The rest follows recommendations from Joshua Bloch on writing good hashCode methods. It would be nice if we had something about the right way (tm) of writing the standard clone(), toString(), hashCode() and equals() in the Hackers Guide. (hint...) I see. I'll try to come up with something based on what I've seen on the web and Bloch's book. If two objects are not equal, they should have different hashCodes. field_attribute is checked in the equality test, so it should play a role in hashCode calculation as well. Should yes, but not must. I was just wondering whether or not the field_attribute really matters since that field and field_id indicate the same property. Having cheap hashCode() methods is a good thing and IMHO field_attribute does not make the hashCode() more specific. But if you think otherwise then please do it as you have it now. True. I'll write some test code, and may post another patch that removes it based on what I find. cheers, dalibor topic Index: java/text/FieldPosition.java === RCS file: /cvsroot/classpath/classpath/java/text/FieldPosition.java,v retrieving revision 1.6 diff -u -r1.6 FieldPosition.java --- java/text/FieldPosition.java19 Nov 2003 17:41:21 - 1.6 +++ java/text/FieldPosition.java23 Nov 2003 20:34:53 - @@ -168,7 +168,7 @@ * p * ul * liThe specified object is not codenull/code. - * liThe specified object is an instance of codeFieldPosition/code. + * liThe specified object has the same class as this object. * liThe specified object has the same field identifier, field attribute * and beginning and ending index as this object. * /ul ___ Classpath mailing list [EMAIL PROTECTED] http://mail.gnu.org/mailman/listinfo/classpath
RE: [PATCH] Field position attribute handling
Dalibor Topic wrote: 7 is the largest prime, that's still an iconst, iconst_7, afaik. That makes it take less bytecode. Actually, iconst_5 is the maximum, but this seems like a little too low level view of the problem space ;-) If two objects are not equal, they should have different hashCodes. No this isn't true, however, if two objects are equal, they must have identical hashCodes. It would be nice if unequal objects had different hashCodes, but this is not a requirement. Regards, Jeroen ___ Classpath mailing list [EMAIL PROTECTED] http://mail.gnu.org/mailman/listinfo/classpath
Re: [PATCH] Field position attribute handling
Jeroen Frijters wrote: Dalibor Topic wrote: 7 is the largest prime, that's still an iconst, iconst_7, afaik. That makes it take less bytecode. Actually, iconst_5 is the maximum, but this seems like a little too low level view of the problem space ;-) Thanks, I'll make it 5, then ;) If two objects are not equal, they should have different hashCodes. No this isn't true, however, if two objects are equal, they must have identical hashCodes. It would be nice if unequal objects had different hashCodes, but this is not a requirement. Note that I used *should* in my sentence instead of *must*, and reparse what I said ;) cheers, dalibor topic ___ Classpath mailing list [EMAIL PROTECTED] http://mail.gnu.org/mailman/listinfo/classpath
Re: [PATCH] Field position attribute handling
Dalibor Topic wrote:
Hi Tom,
thanks for the comment. I agree that's more readable. I'll change that
in another revision.
Done. Please review the next revision, and check in if it's o.k.
2003-11-18 Guilhem Lavaux [EMAIL PROTECTED]
* java/text/FieldPosition.java (field_attribute): New field.
(FieldPosition (Format.Field), FieldPosition(Format.Field, int),
getFieldAttribute): New methods.
2003-11-18 Dalibor Topic [EMAIL PROTECTED]
* java/text/FieldPosition.java (equals): Adapted to handle
field_attribute. Added fast-circuit check for comparison to self.
Replaced use of instanceof by getClass to fix symmetry for derived
types.
(toString): Adapted to handle field_attribute. Improved readability.
(hashCode): New method.
Index: java/text/FieldPosition.java
===
RCS file: /cvsroot/classpath/classpath/java/text/FieldPosition.java,v
retrieving revision 1.5
diff -u -r1.5 FieldPosition.java
--- java/text/FieldPosition.java22 Jan 2002 22:27:01 - 1.5
+++ java/text/FieldPosition.java19 Nov 2003 11:04:54 -
@@ -65,6 +65,38 @@
private int end;
/**
+ * This is the field attribute value.
+ */
+ private Format.Field field_attribute;
+
+ /**
+ * This method initializes a new instance of codeFieldPosition/code
+ * to have the specified field attribute. The attribute will be used as
+ * an id.
+ *
+ * @param field The field format attribute.
+ */
+ public FieldPosition (Format.Field field)
+ {
+this.field_attribute = field;
+ }
+
+ /**
+ * This method initializes a new instance of codeFieldPosition/code
+ * to have the specified field attribute. The attribute will be used as
+ * an id is non null. The integer field id is only used if the Format.Field
+ * attribute is not used by the formatter.
+ *
+ * @param field The field format attribute.
+ * @param field_id The field identifier value.
+ */
+ public FieldPosition (Format.Field field, int field_id)
+ {
+this.field_attribute = field;
+this.field_id = field_id;
+ }
+
+ /**
* This method initializes a new instance of codeFieldPosition/code to
* have the specified field id.
*
@@ -85,6 +117,11 @@
return field_id;
}
+ public Format.Field getFieldAttribute ()
+ {
+return field_attribute;
+ }
+
/**
* This method returns the beginning index for this field.
*
@@ -132,8 +169,8 @@
* ul
* liThe specified object is not codenull/code.
* liThe specified object is an instance of codeFieldPosition/code.
- * liThe specified object has the same field identifier and beginning
- * and ending index as this object.
+ * liThe specified object has the same field identifier, field attribute
+ * and beginning and ending index as this object.
* /ul
*
* @param obj The object to test for equality to this object.
@@ -143,15 +180,40 @@
*/
public boolean equals (Object obj)
{
-if (! (obj instanceof FieldPosition))
+if (this == obj)
+ return true;
+
+if (obj == null || obj.getClass() != this.getClass())
return false;
FieldPosition fp = (FieldPosition) obj;
return (field_id == fp.field_id
+(field_attribute == fp.field_attribute
+ || (field_attribute != null
+field_attribute.equals(fp.field_attribute)))
begin == fp.begin
end == fp.end);
}
+
+ /**
+ * This method returns a hash value for this object
+ *
+ * @return A hash value for this object.
+ */
+ public int hashCode ()
+ {
+int hash = 5;
+
+hash = 31 * hash + field_id;
+hash = 31 * hash + begin;
+hash = 31 * hash + end;
+hash = 31 * hash +
+ (null == field_attribute ? 0 : field_attribute.hashCode());
+
+return hash;
+ }
+
/**
* This method returns a codeString/code representation of this
* object.
@@ -160,7 +222,11 @@
*/
public String toString ()
{
-return (getClass ().getName () + [field= + getField () + ,beginIndex=
- + getBeginIndex () + ,endIndex= + getEndIndex () + ]);
+return (getClass ().getName ()
+ + [field= + getField ()
+ + ,attribute= + getFieldAttribute ()
+ + ,beginIndex= + getBeginIndex ()
+ + ,endIndex= + getEndIndex ()
+ + ]);
}
}
___
Classpath mailing list
[EMAIL PROTECTED]
http://mail.gnu.org/mailman/listinfo/classpath
Re: [PATCH] Field position attribute handling
Dalibor == Dalibor Topic [EMAIL PROTECTED] writes: Dalibor Done. Please review the next revision, and check in if it's o.k. Looks ok to me, I checked it in. Do you not have classpath write access? Tom ___ Classpath mailing list [EMAIL PROTECTED] http://mail.gnu.org/mailman/listinfo/classpath
Re: [PATCH] Field position attribute handling
Dalibor == Dalibor Topic [EMAIL PROTECTED] writes: Dalibor So if you want to offer me write access again, I'd take it ;) I think it would be a good idea. You can make a savannah account yourself, if you don't already have one. Then Mark has to do some admin thing to give you access. Tom ___ Classpath mailing list [EMAIL PROTECTED] http://mail.gnu.org/mailman/listinfo/classpath
Re: [PATCH] Field position attribute handling
Tom Tromey wrote: Dalibor == Dalibor Topic [EMAIL PROTECTED] writes: Dalibor Done. Please review the next revision, and check in if it's o.k. Looks ok to me, I checked it in. Do you not have classpath write access? Nope, not yet ;) Bryce asked back when I sent in my paperwork if I wanted it, and I didn't, as I had no use for it back then. It wasn't clear whether kaffe would switch to GNU Classpath completely, so I didn't want to risk potentially polluting Classpath with GPLd code from kaffe as I wasn't sure how to do the merge properly. Now that I know how to merge stuff properly (drop kaffe's implementation for a class, chuck in Classpath's, review, fix, submit a patch to Classpath) I have no such fear anymore, so that's changed a bit. So if you want to offer me write access again, I'd take it ;) cheers, dalibor topic ___ Classpath mailing list [EMAIL PROTECTED] http://mail.gnu.org/mailman/listinfo/classpath
Re: [PATCH] Field position attribute handling
Tom Tromey wrote: Dalibor == Dalibor Topic [EMAIL PROTECTED] writes: Dalibor So if you want to offer me write access again, I'd take it ;) I think it would be a good idea. You can make a savannah account yourself, if you don't already have one. Then Mark has to do some admin thing to give you access. I've got a savannah account already[1], mostly for filing bugs patches on Classpath ;) cheers, dalibor topic [1] under the unobvious name of robilad. ___ Classpath mailing list [EMAIL PROTECTED] http://mail.gnu.org/mailman/listinfo/classpath
Re: [PATCH] Field position attribute handling
On Wednesday 19 November 2003 11:38, Dalibor Topic wrote: Jeroen Frijters wrote: Dalibor Topic wrote: [...] If two objects are not equal, they should have different hashCodes. No this isn't true, however, if two objects are equal, they must have identical hashCodes. It would be nice if unequal objects had different hashCodes, but this is not a requirement. Note that I used *should* in my sentence instead of *must*, and reparse what I said ;) In general it's impossible to ensure that non-equal objects have different hashcodes - just think for a moment about java.lang.Long. So IMHO even should is going too far ... -- Chris Gray/k/ Embedded Java Solutions Embedded Mobile Java, OSGi http://www.kiffer.be/k/ [EMAIL PROTECTED] +32 477 599 703 ___ Classpath mailing list [EMAIL PROTECTED] http://mail.gnu.org/mailman/listinfo/classpath
Re: [PATCH] Field position attribute handling
Hi Mark,
Mark Wielaard wrote:
Hi,
On Sat, 2003-11-15 at 19:43, Dalibor Topic wrote:
2003-11-15 Guilhem Lavaux [EMAIL PROTECTED]
* java/text/FieldPosition.java (field_attribute): New field.
(FieldPosition (Format.Field), FieldPosition(Format.Field, int),
getFieldAttribute) New methods.
Shouldn't the hashCode(), equals() and possibly toString() methods be
updated to take the new field into account?
Thanks for pointing that out. Please take a look at the updated,
attached patch, and check it in.
2003-11-18 Guilhem Lavaux [EMAIL PROTECTED]
* java/text/FieldPosition.java (field_attribute): New field.
(FieldPosition (Format.Field), FieldPosition(Format.Field, int),
getFieldAttribute) New methods.
2003-11-18 Dalibor Topic [EMAIL PROTECTED]
* java/text/FieldPosition.java (equals) : Adapted to handle
field_attribute. Added fast-circuit check for comparison to self.
Replaced use of instanceof by getClass to fix symmetry for derived
types.
(toString) Adapted to handle field_attribute. Improved readability.
(hashCode) New method.
Index: java/text/FieldPosition.java
===
RCS file: /cvsroot/classpath/classpath/java/text/FieldPosition.java,v
retrieving revision 1.5
diff -u -r1.5 FieldPosition.java
--- java/text/FieldPosition.java22 Jan 2002 22:27:01 - 1.5
+++ java/text/FieldPosition.java18 Nov 2003 18:42:32 -
@@ -65,6 +65,38 @@
private int end;
/**
+ * This is the field attribute value.
+ */
+ private Format.Field field_attribute;
+
+ /**
+ * This method initializes a new instance of codeFieldPosition/code
+ * to have the specified field attribute. The attribute will be used as
+ * an id.
+ *
+ * @param field The field format attribute.
+ */
+ public FieldPosition (Format.Field field)
+ {
+this.field_attribute = field;
+ }
+
+ /**
+ * This method initializes a new instance of codeFieldPosition/code
+ * to have the specified field attribute. The attribute will be used as
+ * an id is non null. The integer field id is only used if the Format.Field
+ * attribute is not used by the formatter.
+ *
+ * @param field The field format attribute.
+ * @param field_id The field identifier value.
+ */
+ public FieldPosition (Format.Field field, int field_id)
+ {
+this.field_attribute = field;
+this.field_id = field_id;
+ }
+
+ /**
* This method initializes a new instance of codeFieldPosition/code to
* have the specified field id.
*
@@ -85,6 +117,11 @@
return field_id;
}
+ public Format.Field getFieldAttribute ()
+ {
+return field_attribute;
+ }
+
/**
* This method returns the beginning index for this field.
*
@@ -132,8 +169,8 @@
* ul
* liThe specified object is not codenull/code.
* liThe specified object is an instance of codeFieldPosition/code.
- * liThe specified object has the same field identifier and beginning
- * and ending index as this object.
+ * liThe specified object has the same field identifier, field attribute
+ * and beginning and ending index as this object.
* /ul
*
* @param obj The object to test for equality to this object.
@@ -143,15 +180,39 @@
*/
public boolean equals (Object obj)
{
-if (! (obj instanceof FieldPosition))
+if (this == obj)
+ return true;
+
+if (obj != null (obj.getClass() != this.getClass()))
return false;
FieldPosition fp = (FieldPosition) obj;
return (field_id == fp.field_id
+(field_attribute == fp.field_attribute
+ || (field_attribute != null
+field_attribute.equals(fp.field_attribute)))
begin == fp.begin
end == fp.end);
}
+
+ /**
+ * This method returns a hash value for this object
+ *
+ * @return A hash value for this object.
+ */
+ public int hashCode ()
+ {
+int hash = 7;
+
+hash = 31 * hash + field_id;
+hash = 31 * hash + begin;
+hash = 31 * hash + end;
+hash = 31 * hash + (null == field_attribute ? 0 : field_attribute.hashCode());
+
+return hash;
+ }
+
/**
* This method returns a codeString/code representation of this
* object.
@@ -160,7 +221,11 @@
*/
public String toString ()
{
-return (getClass ().getName () + [field= + getField () + ,beginIndex=
- + getBeginIndex () + ,endIndex= + getEndIndex () + ]);
+return (getClass ().getName ()
+ + [field= + getField ()
+ + ,attribute= + getFieldAttribute ()
+ + ,beginIndex= + getBeginIndex ()
+ + ,endIndex= + getEndIndex ()
+ + ]);
}
}
___
Classpath mailing list
[EMAIL PROTECTED]
http://mail.gnu.org/mailman/listinfo/classpath
Re: [PATCH] Field position attribute handling
On Tue, Nov 18, 2003 at 08:30:44PM +0100, Dalibor Topic wrote: 2003-11-18 Guilhem Lavaux [EMAIL PROTECTED] * java/text/FieldPosition.java (field_attribute): New field. (FieldPosition (Format.Field), FieldPosition(Format.Field, int), getFieldAttribute) New methods. 2003-11-18 Dalibor Topic [EMAIL PROTECTED] * java/text/FieldPosition.java (equals) : Adapted to handle field_attribute. Added fast-circuit check for comparison to self. Replaced use of instanceof by getClass to fix symmetry for derived types. (toString) Adapted to handle field_attribute. Improved readability. (hashCode) New method. There are some missing ':'. The rule is: (methodname): description of change. Michael ___ Classpath mailing list [EMAIL PROTECTED] http://mail.gnu.org/mailman/listinfo/classpath
Re: [PATCH] Field position attribute handling
Michael Koch wrote: On Tue, Nov 18, 2003 at 08:30:44PM +0100, Dalibor Topic wrote: 2003-11-18 Guilhem Lavaux [EMAIL PROTECTED] * java/text/FieldPosition.java (field_attribute): New field. (FieldPosition (Format.Field), FieldPosition(Format.Field, int), getFieldAttribute) New methods. 2003-11-18 Dalibor Topic [EMAIL PROTECTED] * java/text/FieldPosition.java (equals) : Adapted to handle field_attribute. Added fast-circuit check for comparison to self. Replaced use of instanceof by getClass to fix symmetry for derived types. (toString) Adapted to handle field_attribute. Improved readability. (hashCode) New method. There are some missing ':'. The rule is: (methodname): description of change. thnaks for catching that one. how about: 2003-11-18 Guilhem Lavaux [EMAIL PROTECTED] * java/text/FieldPosition.java (field_attribute): New field. (FieldPosition (Format.Field), FieldPosition(Format.Field, int), getFieldAttribute): New methods. 2003-11-18 Dalibor Topic [EMAIL PROTECTED] * java/text/FieldPosition.java (equals): Adapted to handle field_attribute. Added fast-circuit check for comparison to self. Replaced use of instanceof by getClass to fix symmetry for derived types. (toString): Adapted to handle field_attribute. Improved readability. (hashCode): New method. cheers, dalibor topic ___ Classpath mailing list [EMAIL PROTECTED] http://mail.gnu.org/mailman/listinfo/classpath
Re: [PATCH] Field position attribute handling
Hi,
On Tue, 2003-11-18 at 20:30, Dalibor Topic wrote:
* liThe specified object is an instance of codeFieldPosition/code.
- * liThe specified object has the same field identifier and beginning
- * and ending index as this object.
+ * liThe specified object has the same field identifier, field attribute
+ * and beginning and ending index as this object.
* /ul
*
* @param obj The object to test for equality to this object.
@@ -143,15 +180,39 @@
*/
public boolean equals (Object obj)
{
-if (! (obj instanceof FieldPosition))
+if (this == obj)
+ return true;
+
+if (obj != null (obj.getClass() != this.getClass()))
return false;
The documentation and code don't match up now.
My Class Library book seems to indicate that it actually is instanceof,
thought in most cases your code comparing the actual Class seems more
correct.
+ /**
+ * This method returns a hash value for this object
+ *
+ * @return A hash value for this object.
+ */
+ public int hashCode ()
+ {
+int hash = 7;
+
+hash = 31 * hash + field_id;
+hash = 31 * hash + begin;
+hash = 31 * hash + end;
+hash = 31 * hash + (null == field_attribute ? 0 : field_attribute.hashCode());
+
+return hash;
+ }
Just curious, why this particular hash function?
I couldn't find one specified, so I guess any good one will do.
Don't know if you need to include the field_attribute.hashCode() since
it will probably depend on the field_id in almost all cases.
Cheers,
Mark
signature.asc
Description: This is a digitally signed message part
___
Classpath mailing list
[EMAIL PROTECTED]
http://mail.gnu.org/mailman/listinfo/classpath
Re: [PATCH] Field position attribute handling
Dalibor == Dalibor Topic [EMAIL PROTECTED] writes: Dalibor -if (! (obj instanceof FieldPosition)) Dalibor +if (this == obj) Dalibor + return true; Dalibor + Dalibor +if (obj != null (obj.getClass() != this.getClass())) Daliborreturn false; I think this should read: if (obj == null || obj.getClass() != this.getClass()) return false; I don't think short-circuiting the this==obj case is really worth the effort. But I don't really care all that much. Dalibor +hash = 31 * hash + (null == field_attribute ? 0 : field_attribute.hashCode()); Does this line need to be wrapped? I think it goes past column 79. Tom ___ Classpath mailing list [EMAIL PROTECTED] http://mail.gnu.org/mailman/listinfo/classpath
Re: [PATCH] Field position attribute handling
Hi Mark,
Mark Wielaard wrote:
The documentation and code don't match up now.
My Class Library book seems to indicate that it actually is instanceof,
thought in most cases your code comparing the actual Class seems more
correct.
It's the recommended way of doing equals() if you want to avoid symmetry
bugs. See PRAXIS 12 in Peter Haggar's Practical Java, or §5.1, p 119 in
Java 2 performance and idion guide by craig lahrmann rhett guthrie, or
Effective Java by Joshua Bloch. Using instanceof is a bad idea because
it allows for symmetry bugs:
B extends A. With instanceof in equals, we can have A.equals(B) but
!B.equlas(A) since A is not an instance of B. Ugly, and very broken.
There are a few places, where the API demands this (Maps), but they are
all well documented, AFAIK. If it's not documented, it's better to use
the symmetry safe form, in my opinion.
+ /**
+ * This method returns a hash value for this object
+ *
+ * @return A hash value for this object.
+ */
+ public int hashCode ()
+ {
+int hash = 7;
+
+hash = 31 * hash + field_id;
+hash = 31 * hash + begin;
+hash = 31 * hash + end;
+hash = 31 * hash + (null == field_attribute ? 0 : field_attribute.hashCode());
+
+return hash;
+ }
Just curious, why this particular hash function?
7 is the largest prime, that's still an iconst, iconst_7, afaik. That
makes it take less bytecode.
The rest follows recommendations from Joshua Bloch on writing good
hashCode methods.
I couldn't find one specified, so I guess any good one will do.
Don't know if you need to include the field_attribute.hashCode() since
it will probably depend on the field_id in almost all cases.
If two objects are not equal, they should have different hashCodes.
field_attribute is checked in the equality test, so it should play a
role in hashCode calculation as well.
cheers,
dalibor topic
___
Classpath mailing list
[EMAIL PROTECTED]
http://mail.gnu.org/mailman/listinfo/classpath
Re: [PATCH] Field position attribute handling
Hi Tom, Tom Tromey wrote: Dalibor == Dalibor Topic [EMAIL PROTECTED] writes: Dalibor -if (! (obj instanceof FieldPosition)) Dalibor +if (this == obj) Dalibor + return true; Dalibor + Dalibor +if (obj != null (obj.getClass() != this.getClass())) Daliborreturn false; I think this should read: if (obj == null || obj.getClass() != this.getClass()) return false; thanks for the comment. I agree that's more readable. I'll change that in another revision. I don't think short-circuiting the this==obj case is really worth the effort. But I don't really care all that much. A quick grep for equals in java.util.* collection classes shows that it's being used quite a bit there. for example, in order to find out whether an element is contained in a collection, on needs to find out whether there is an element that is equal to it in the collection. I believe that a short circuit here helps. I could run some tests and provide some numbers, if necessary. Dalibor +hash = 31 * hash + (null == field_attribute ? 0 : field_attribute.hashCode()); Does this line need to be wrapped? I think it goes past column 79. can't say, I need to teach emacs to count columns, I guess. But I'll wrap it anyway, to go for sure. cheers, dalibor topic ___ Classpath mailing list [EMAIL PROTECTED] http://mail.gnu.org/mailman/listinfo/classpath
Re: [PATCH] Field position attribute handling
Hi, On Sat, 2003-11-15 at 19:43, Dalibor Topic wrote: 2003-11-15 Guilhem Lavaux [EMAIL PROTECTED] * java/text/FieldPosition.java (field_attribute): New field. (FieldPosition (Format.Field), FieldPosition(Format.Field, int), getFieldAttribute) New methods. Shouldn't the hashCode(), equals() and possibly toString() methods be updated to take the new field into account? Cheers, Mark signature.asc Description: This is a digitally signed message part ___ Classpath mailing list [EMAIL PROTECTED] http://mail.gnu.org/mailman/listinfo/classpath
