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?
> 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 have some comments on your code, some of them more nitpicky than
others.
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.
> #define NORMAL_BITS 7
> #define LENGTH_BITS 5
> -
Should probably keep this blank line.
> /* ----- 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.
> +/* 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.
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.
> + /* 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?
> +/* 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.
> + if (cache)
> + {
> + svn_pool_destroy (cache->pool);
> + }
We don't generally put braces around a single-line body of a control
statement.
> + /* 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?
> + apr_off_t address;
> + int m;
> + apr_off_t currpos = cache->pos;
> + int mode;
Blank line after declarations, please.
> + /* 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.:
/* 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.
---------------------------------------------------------------------
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