[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: rev 1752 - trunk/subversion trunk/subversion/include trunk/subversion/libsvn_diff

From: Philip Martin <philip_at_codematters.co.uk>
Date: 2002-04-25 15:12:17 CEST

"Sander Striker" <striker@apache.org> writes:

> > > +/*
> > > + * Support structure to implement 'large arrays'.
> > > + */
> >
> > This is some sort of optimisation isn't it? Is there any profiling
> > that shows a) that it is needed, or b) that it works?
>
> Like I said in my post to the list; we need to investigate if we
> can solve this with basic apr types. This was my implementation
> to get something to work quickly.

A comment in the code is better than a email, then when I look at the
code I won't have to ask questions.

> [...]
> >> + newhat = apr_pcalloc(pool, sizeof(svn_diff_hat_t) + 4095 * sizeof(svn_diff_lcs_t *));
> >
> > A magic number! Why? How did you choose it?
>
> I drew a magic circle on the floor, lit the candles, danced until it started to
> rain and then decided on 4096 (-1 for the one already in the struct). This
> is one of the things that would disappear entirely if we can use an apr type
> for this. /me wonders if apr_array would work.
>
> [...]
> > > + size = 4096;
> >
> > Another one, I assume it's related to the previous one...
>
> Yes.

You seem to have missed my point. Putting one such number in the code
is barely acceptable, putting two different but related numbers is
wrong. There should be a single constant #define'd. This should be
accompanied by a comment saying '4096 because ....' or 'this is just a
guess'. How is anyone else supposed to know what is important if you
don't document it?

-- 
Philip
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Apr 25 15:13:27 2002

This is an archived mail posted to the Subversion Dev mailing list.