On Fri, Feb 5, 2010 at 04:31, Julian Foad <julian.f...@wandisco.com> wrote: > On Fri, 2010-02-05 at 03:01 +0100, Neels J Hofmeyr wrote: >> In this commit, I run svn_wc__db_read_info() in check_tree_conflicts(), when >> half or more of its callers have already called svn_wc__db_read_info(). >> >> So would it be an optimization to pass STATUS, maybe more, as args to >> check_tree_conflict(), rather than calling svn_wc__db_read_info() twice? > > Let's do a mental experiment. Think about what it would be like if you > chose to pass several of the values that had already been read from the > database. That would start to get messy if they were all separate > params, so we would build a structure that held the values, and pass a > pointer to it. We could generalize the structure to hold all possible > __db_read_info() data, with each value optionally present or not (marked > by an invalid value). It would effectively be a cache. It could also > hold a pointer to the DB it came from, so we wouldn't need to pass that > separately, and we could call the pointer to the struct "a handle to the > DB". See where I'm going? > > It seems to me that we should rely on svn_wc__db_read_info() to be > efficient. If it needs caching to be fast, it should implement the > equivalent of that hypothetical struct internally.
Yes, this is the intent behind the API design. We're create optimizations internally when possible, and expose more APIs/data when we cannot. The "many params" style is also to avoid passing structures that are simply gloms of parameters. That results in a style of coding where you cannot tell whether any particular value is necessary, and (in the case of entry_modify) whether any given value is actually *present* (look at flag bits!). It ends up making the system very opaque and hard to analyze the data flows: who needs what. You may also note that we've already added a simplified version of read_info() called read_kind(). We may end up with one or two more, once we're done and we see the usage patterns. (tho, honestly, ref: read_kind, I'm still a bit queasy with it returning node_unknown, but we can eval that later) On the larger topic of passing-params-via-struct... I am a firm believer against that. ISTR that the merge_baton was constructed for this very purpose. And again, it makes it very difficult to reason about the operation of the merge code. I also note that it tends to create massive functions in the merge code, rather than smaller functions taking targeted parameters. If somebody wants to nuke that "pattern" of coding, then I will personaly buy you several beers (or some alternative drink of your choice). Cheers, -g
