On Tue, Sep 30, 2008 at 4:27 PM, John Tamplin <[EMAIL PROTECTED]> wrote:
> 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 >> > > 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. > Or you'd have to introduce a new static method, and call that from somewhere "early" (i.e. after we have a TreeLogger, but before we're doing anything that might loop over calls). > > > - 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? > Nevermind. :-/ That answers it; I just misread. > >> - 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)); > Yes, I like this better than the set-to-default-int, fetch-property, test-for-null sequence. Though my formulation had converted default to String, rather than using String.valueOf(), which gets us to your next comment... 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. > There's a reason it's in the "nits" category, and if you think it's a lose, I don't mind if you skip the suggestion. But, to make the case: I see one all-String code path as cleaner than two paths with different data data types on each fork. I'll grant that the integer-fork "else" of your "if" is degenerate, but it takes a couple bytes of code size to do the test. Equally, surely it's not a real problem to ensure that the compiled-in default is in fact an integer (that is, the used-once compiled-in string's type safety is a small issue, even among small issues), and we need to handle non-integer values gracefully anyway (because the user can override). But, again, it's a nit: my way saves at most ~10b of code size and could gain or lose a few miliseconds in execution time depending on whether branch prediction in the JVM is so far below bytecode interpretation that misses appear free; your way is type-safe for a constant that's both clearly documented as an integral value and likely to change exactly once in its coded lifetime. What color dresses are the angels on that pin wearing? --~--~---------~--~----~------------~-------~--~----~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~----------~----~----~----~------~----~------~--~---
