On Wed, Jun 13, 2001 at 06:18:11PM -0500, Ben Collins-Sussman wrote:
> Greg Stein <gstein@lyra.org> writes:
>...
> > This isn't going to work well.
>
> And Ben rebuts! Karl spent a long time arguing about how to solve
> this problem. We were waiting for someone to attack. ;)
Bark! Bark!
> > If the first person to call svn_strerror() passes a subpool, then future
> > callers will get garbaged if that pool has gone away. It is fixable by using
> > a cleanup so the hash can be rebuilt for another caller.
>
> There is only one caller: svn_handle_error, which always uses the
> pool within the svn_error_t, which is the top-level pool.
That argument doesn't hold. There is one caller *now*. svn_handle_error is
only appropriate for command line clients, and even then... it is rather
inflexible in how it processes errors.
As soon as you get another caller, the argument about "one caller"
evaporates :-)
> So -- we either need to make svn_strerror a static routine (so that
> ONLY svn_handle_error can use it), or we need to document that the
> pool must be top-level pool, or we need
Documenting is a way around the basic problem: if the pool goes away, then
the hash table gets garbaged. As I mentioned, you could have a cleanup that
stores a NULL into the static variable. Next time somebody goes for an
error, the hash table gets rebuilt again, using whatever pool was passed at
that point in time.
> > However, this is also overengineered. We don't need a hash table since this
> > function won't be called often. A simple linear scan over a table is
> > sufficient. That would allow constant data to be used for the whole table.
>
> You miss the point of the hash table.
No, I didn't :-) I understand that the hash table can have "holes" and that
it is immune to ordering. You missed the point of my comment (aka I didn't
explain enough).
I said "linear scan over a table". That implies you're doing a search --
searching for the error code.
You assumed a table indexed by the error code. You wouldn't need a scan for
that one :-) ... and yes, that kind of table can be busted six ways from
Tuesday.
> We expect a lot of skew. At first, we thought, "let's avoid the skew"
> by forcing all new svn errors & descriptions to be put into a single
> python list -- and then at build time, python can output a mini .h and
> .c file to be #included in svn_error.h and svn_error.c. (This is what
> a lot of big projects do.)
Urk. Over-engineered again :-). Although... a system similar to that can be
very handy for i18n. We may want to investigate the best way to do that when
we do the i18n stuff.
>...
> The point of having a hash is that order doesn't matter. Either of
> the two mistakes don't matter. svn_strerror() just returns "no
> description available" if the code isn't in the hash. And it doesn't
> matter where (in the source) the new description is hash-set.
Sure. Same with my table idea. But I can also guarantee that a person won't
forget a description, and I can avoid the heap, and I can avoid pool
cleanups, and ... :-)
> > And lastly, it would be nice to just call svn_strerror() and have it grab
> > the APR error for us, if it falls in the APR range. No need for callers to
> > know everything about the ranges.
>
> We can do that, easily. That logic is currently in svn_handle_warning
> right now, but we can move it.
The other benefit here, is using the signature of apr_strerror(). By saying
that the error function *writes* into a provided buffer, it is possibly to
construct an error and return that. There are scaling issues if you
construct error strings on the heap, so the current "const char *" pattern
can really only look up predefined, constant strings.
This doesn't matter to us now, but it can for the future, and (certainly) if
we defer to apr_strerror(), then the signature will be handy.
> > I've got a design for rectifying all of this, and can implement tonite. I
> > explained the evil macros to Karl if you can't wait to see the code
> > :-)
>
> Oh, oops. Have you already talked Karl out of this system? Then I
> guess my comments are irrelevant. :-)
Not at all. We talked about it last month when we was out this way. I only
described it. That would hardly be talking anybody into anything :-)
> > [ the gist is to have a header that can create both the enumeration and the
> > error string table, depending on how it is included ]
>
> Whoa. My head hurts.
hehe. I'll check in the header and example code. If people agree with it,
then we can hook it in. If not, then it can be deleted. But I'm out for the
weekend, so you can yak about it (or install it) while I'm gone.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
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:36:31 2006