http://gwt-code-reviews.appspot.com/1601805/diff/1/user/src/com/google/web/bindery/autobean/shared/ValueCodex.java
File user/src/com/google/web/bindery/autobean/shared/ValueCodex.java
(right):
http://gwt-code-reviews.appspot.com/1601805/diff/1/user/src/com/google/web/bindery/autobean/shared/ValueCodex.java#newcode106
user/src/com/google/web/bindery/autobean/shared/ValueCodex.java:106: if
(value.isNumber()) {
On 2011/12/06 01:54:28, skybrian wrote:
This seems fine, but could you add a test to AutoBeanCodexTest to make
sure it
continues to work?
Done.
http://gwt-code-reviews.appspot.com/1601805/diff/1/user/src/com/google/web/bindery/autobean/shared/ValueCodex.java#newcode114
user/src/com/google/web/bindery/autobean/shared/ValueCodex.java:114:
long timestamp = ((Date) value).getTime();
On 2011/12/06 01:54:28, skybrian wrote:
As I said in the bug, I'm not sure this is a good idea.
And as I said, I tend to agree. I've thus reverted that change.
http://gwt-code-reviews.appspot.com/1601805/diff/1/user/src/com/google/web/bindery/autobean/shared/ValueCodex.java#newcode368
user/src/com/google/web/bindery/autobean/shared/ValueCodex.java:368: if
(clazz.isEnum()) {
On 2011/12/06 01:54:28, skybrian wrote:
On 2011/12/01 17:18:36, tbroyer wrote:
> Oh, I don't know why I had that in this CL, but the rationale is: in
the only
> case where findType can be called with an
"enum-value-with-overridden-methods'
> class" (a subclass of the isEnum class), it's OK for findType to
return null
> because the value will be "upcasted" (and ENUM.canUpcast will then
return
true,
> so it'll correctly be used); see encode(Object) above.
> So the question is whether to add an overhead for every encoded
value
(checking
> getSuperClass.isEnum here) or only for those specific enum values
(loop over
the
> types and ask them if they canUpcast the value). I'd rather say the
latter,
> hence this change.
It looks like you're right that the answer will still be correct, but
I'm not
sure about the performance implications, so I'd be inclined to leave
it as is
unless you've done some performance testing showing that it's a win.
I haven't done any testing (yet), but the perf implications seem rather
obvious to me (not how *much* perf would be impacted though): for
*every* type other than an enum, clazz.isEnum() will be false, so
"clazz.getSuperclass() != null && clazz.getSuperclazz().isEnum()" will
be tested (and will return false for Strings, Dates, and primitives and
their wrappers).
There's only a rare case where this test is useful: an enum value "with
a body" (such as AutoBeanCodexTest.MyEnum.BAR).
MyEnum.BAR.getClass().isEnum() is false, but getSuperclass() returns
MyEnum.class where isEnum() is true. If you remove the test on the
super-class, then for such an enum value it'll fallback to
TYPES_BY_CLASS.get() which will be null (then, when called from
encode(Object), it'll then loop over the Type.values() until it finds
Type.ENUM, whose canUpcast would return true).
There are two cases that this change would break:
encode(MyEnum.BAR.getClass(), MyEnum.BAR) and
decode(MyEnum.BAR.getClass(), StringQuoter.quote("BAR"))
(passing MyEnum.BAR.getClass() instead of MyEnum.class)
but they're no much different than encode(java.sql.Date.class, someDate)
vs. encode(java.util.Date.class, someDate); and note also that
AbstractAutoBeanFactory.getEnum() would fail too with
MyEnum.BAR.getClass() as argument.
Doesn't the 80/20 rule apply? (and it's rather 98/2 here)
I'm OK to revert the change though, and possibly reintroduce it later
after some perf testing.
(I'll try to do that testing right now though, so we can have numbers on
which to base our decision).
http://gwt-code-reviews.appspot.com/1601805/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
