I'm still not crazy about having addClientData() and getClientData() on separate objects. It seems to me that you've violated your own principal that the GeneratorContext should be the only object that has to get passed to the generator's helpers.
Can addClientData() move to the context? That would require getClientData() to be able to retrieve both cached and freshly added data. Is that practical? Sounds like it would be hugely convenient. On Wed, Aug 3, 2011 at 12:43 PM, <jbrosenb...@google.com> wrote: > Some responses. I did consider most of your suggestions, and in fact > for a while I did have things as you suggest, so great minds think alike > :). I'll outline my reasoning: > > First, IncrementalGenerators have to live in the same ecosystem as all > the currently extent non-incremental Generator implementations. Having > a uniform rebind process whereby the StandardRebindOracle is agnostic to > this greatly simplified things. If we were starting from scratch, I > think it would probably look quite a bit different. > > Standard Generators only return a type-name (which can possibly be > null). The RebindResult allows legacy generators to be cleanly wrapped > and presented to the rebind oracle in a uniform way. > > A RebindResult is not the same as a CachedRebindResult. The > StandardRebindOracle uses the info in the RebindResult to decide how to > proceed. It may or may not choose to construct a cache entry, and that > cached entry might contain a partial subset of newly generated types and > types from a previous cache entry. Since a generator itself shouldn't > need to know how to integrate cached types and artifacts, this logic is > better left up to the StandardRebindOracle. The RebindResult also > contains a target type name, and client data, so it's just a container > to wrap a generators results. > > In the case that an IncrementalGenerator decides it can return quickly > and request that all previously cached output can be reused, it doesn't > need to do anything, including not re-committing cached compilation > units and artifacts to the context, etc. That's all magic handled by > the rebind oracle. > > A CachedRebindResult is specifically input to an IncrementalGenerator, > but it's cumbersome to pass it in as a separate arg to > generateIncrementally. Having it live in the GeneratorContext makes one > less thing a generator implementation needs to pass around to it's > internal methods (since GeneratorContext at the least is commonly needed > all over the place for a generator anyway). > > Remember too, that much of the time, an IncrementalGenerator will run > when generatorResultCaching is not enabled, such as currently for all > web-mode compiles, so it's more of a contextual thing anyway, which > makes it more intuitive for it to live in the GeneratorContext. > > I did have a separate GeneratContextExt interface, which extended the > base GeneratorContext interface, but ultimately this became unnecessary, > and it greatly simplified the framework by unifying things. > > I think you had a couple other comments offline (but not summarized > above), I'll wait for your further comments to respond to those. > > > http://gwt-code-reviews.**appspot.com/1468804/<http://gwt-code-reviews.appspot.com/1468804/> > -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
