On 14 Feb 2002, Greg Hudson wrote:
> On Thu, 2002-02-14 at 19:20, Daniel Berlin wrote:
> > For a single random-test run (upped to 300 iterations rather than 30), we
> > usually save 58k over all the files it creates.
>
> What percent savings is this relative to svndiff 0?
It varies, but it can be upwards of 50%, actually.
Averages to 15%.
>
> > The standard cache size is 4, 2 not 4, 3 (ie near table size == 4, same
> > table size = 2)
>
> I can't decipher this sentence.
I specifically said these are changes relative to vcdiff, and vcdiff
specifically says that it defines a default code table where size of near
table == 4, size of same table == 3.
This is changed to be 4, 2.
>
> I have some comments on your code, some of them more nitpicky than
> others.
Errr, I didn't want it reviewed on nitpicky issues or commenting issues
because i'm not finished with that stuff yet.
The implementation itself is what i was looking for any comments on (if
any).
>
> One general note is that your code uses tabs. I think most Subversion
> code doesn't (svn-dev.el sets indent-tab-mode to nil) so that diffs look
> pretty and stuff.
I load svn-dev.el before working on .c files, so i'm a bit confused on
this one.
>
> > #define NORMAL_BITS 7
> > #define LENGTH_BITS 5
> > -
>
> Should probably keep this blank line.
Yes, i know.
>
> > /* ----- Text delta to svndiff ----- */
> > -
>
> And this one, and add another after the typedef. Or better yet, don't
> use a typedef; just call your structure "struct svn_cache" and use it
> that way. That's consistent with the style of this file and of the way
> many other files deal with internal-use structures.
Okey.
>
> > +/* Structure of the svndiff v1 cache */
> > +struct svd_cache_s
> > +{
> > + /* Near table */
> > + int near_size;
> > + apr_off_t *near;
> > + int near_index;
>
> This comment nicely categorizes the fields but tells the reader nothing
> about what the table is. This is probably a good place to precisely
> define with the near table keeps track of.
I know.
>
> Unless you have a compelling reason to do otherwise, use arrays for the
> near and same tables. They're small and always the same size; it will
> save significantly on code complexity.
What code complexity? It just saves you about 4 lines in terms of
allocation.
Plus, svndiff2 might use a different standard cache size. Just because
they are currently only in use by one version with one set of parameters
does not mean it will never change.
Making them fixed size arrays does nothing but make it more inflexible.
Unless you want me to make all the cache code specific to v1, and assume
there will never be a v2 that uses tables.
It would also mean one could not reuse the tables for a vcdiff
implementation, which you could right now.
Since a vcdiff implementation for XML output is listed on the todo, i
figured why make it harder?
>
> > + /* Same table */
>
> Same criticism.
>
> > - apr_pool_t *pool;
> > + unsigned char version;
> > + apr_pool_t *pool;
>
> Gratuitous addition of two trailing spaces to the end of the pool
> declaration?
Huh?
>
> > +/* Encoding modes for svndiff v1. Can't be an enum since it needs to
> > + take into account the near and same modes. */
> > +#define SVNDIFF_SELF 0
> > +#define SVNDIFF_HERE 1
>
> You could still, of course, define the constants with an enum
> declaration. ("enum { SVNDIFF_SELF = 0, SVNDIFF_HERE = 1 };".) But
> that's up to you. It's accurate to say that the mode values themselves
> can't have an enum type, of course.
>
> Your comments would be a little clearer, or at least tighter, if you
> stuck to complete sentences instead of sentence fragments.
I know.
>
> > + if (cache)
> > + {
> > + svn_pool_destroy (cache->pool);
> > + }
>
> We don't generally put braces around a single-line body of a control
> statement.
I'm used to gcc, where we do. Since svn-dev used gnu style, i assumed
that unless documented elsewhere, svn code does.
>
> > + /* Create a new pool for this cache. We could just let the user
> > + pass in a pool, but we only make one or two allocations (literally
> > + one or two) total. In reality, it doesn't even make sense to use
> > + a pool here, i'm doing it for consistency. */
> > +
> > + pool = svn_pool_create (NULL);
>
> Not really kosher, I don't think.
>
> Can you just stuff the cache data into the baton, instead of having two
> separately allocated structures?
It didn't seem to fit with the other things in the baton.
But, i'll move it in there just for fun.
>
> > + apr_off_t address;
> > + int m;
> > + apr_off_t currpos = cache->pos;
> > + int mode;
>
> Blank line after declarations, please.
As you wish.
>
> > + /* First, get our address + mode. */
> > + buffer = decode_int (&address, buffer, end);
> > + /* Mask out the mode. */
> > + mode = (address & SVNDIFF_MODE_MASK);
> > + /* And shift out the address. */
> > + address >>= SVNDIFF_MODE_BITS;
>
> The highly literal "play by play" style of comment is, at least in my
> opinion, less useful than more concise comments which explain the intent
> of several logically related lines of code, e.g.:
It was a reminder to myself for when i do that, actually.
As I said, i didn't submit it for review because i'm not finished with the
comments and whatnot, just the implementation and testing.
>
> /* Decode the address and mode. */
> buffer = decode_int (&address, buffer, end);
> mode = (address & SVNDIFF_MODE_MASK);
> address >>= SVNDIFF_MODE_BITS;
>
> > + /* For the self mode, we don't need to do anything further. The
> > + address is encoded as itself. */
>
> When you have an if/else-if block like this, if you put the comments
> into the bodies of the control statements, they won't split up the
> control statement as much.
Yes yes, i know.
--Dan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Oct 21 14:37:08 2006