On Tue, Sep 30, 2008 at 3:49 PM, Freeland Abbott <[EMAIL PROTECTED]
> wrote:
> Mostly LGTM. The only (marginally) substantive complaint is that it may
> make sense to rework the property parsing around lines 285-293 so that it's
> done once, statically, rather than re-parsing the String->int conversion for
> every TypeSerializer we create, especially if (as I suggest as a nit) you
> change to use the System.getProperty(String propname, String default)
> formulation. Not that parsing a short string into int is any large amount
> lot of work, but we may end up doing it often...
>
Ok. I can't really do it in a static intiializer since I may need a
TreeLogger, but I can only do it the first time.
> Nits
>
> - Can we note, probably in the comments around lines 58-66, that the
> shard size is measured in number of reachable types, not in e.g.
> characters? (Yeah, I know characters would be hard to get at *a priori
> *, but since our issue seems to be string length of the JSNI body,
> that's what I expected size to be measured in.)
>
> The comment for DEFAULT_CREATEMETHODMAP_SHARD_SIZE currently says:
> Default number of types to split createMethodMap entries into. Zero means
> no sharding occurs.
>
How are you suggesting that be rewritten?
>
> - Also mentioned above, line 285-286, why avoid the
> System.getProperty(String prop, String default) formulation? (You'd have
> to
> make the default a string, yes, but it saves you the if-null test.)
>
> So you would prefer doing:
String shardSizeProperty =
System.getProperty(GWT_CREATEMETHODMAP_SHARD_SIZE,
String.valueOf(DEFAULT_CREATEMETHODMAP_SHARD_SIZE));
try {
shardSize = Integer.valueOf(shardSizeProperty);
if (shardSize < 0) {
logger.log(TreeLogger.ERROR, GWT_CREATEMETHODMAP_SHARD_SIZE
+ " must be positive: " + shardSizeProperty);
throw new UnableToCompleteException();
}
} catch (NumberFormatException e) {
logger.log(TreeLogger.ERROR, "Property "
+ GWT_CREATEMETHODMAP_SHARD_SIZE + " not a number: "
+ shardSizeProperty, e);
throw new UnableToCompleteException();
}
This would always require converting the default to a string (or storing it
as a string and losing type safety) and always parsing the result rather
than a test telling you the parsing is needed. It doesn't seem to be an
improvement to me.
>
> - I'll take your word for the style-correctness of the added space on
> line 190.
>
> Ctrl-Shift-F formats it that way, so unless my format settings are messed
up that should be the standard.
--
John A. Tamplin
Software Engineer (GWT), Google
--~--~---------~--~----~------------~-------~--~----~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~----------~----~----~----~------~----~------~--~---
