You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Greg Stein <gs...@lyra.org> on 2001/06/13 22:31:33 UTC
Re: CVS update: MODIFIED: libsvn_subr ...
On Tue, Jun 12, 2001 at 08:48:58PM -0000, sussman@tigris.org wrote:
> User: sussman
> Date: 01/06/12 13:48:58
>
> Modified: subversion/libsvn_subr svn_error.c
> Log:
> * svn_error.c (initialize_svn_error_descriptions): new func - build a
> hash table that maps svn errorcodes to english descriptions.
>
> (svn_strerror): take a pool instead of stringbuf. pass
> this pool to initialize_svn_error_descriptions() the first time this
> routine is called, and then cache the hash in a static variable
> thereafter. use the hash for lookups.
>
> (svn_error_strings[]): removed, now part of hash initialization.
This isn't going to work well.
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.
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.
Also... the disconnect between the enumeration in the header and the
definitions in the .c file will cause skew. No matter how much we may
document, somebody will insert an error at the top/bottom/middle and won't
see any comments we leave behind.
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.
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 :-)
[ the gist is to have a header that can create both the enumeration and the
error string table, depending on how it is included ]
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
Re: CVS update: MODIFIED: libsvn_subr ...
Posted by Greg Stein <gs...@lyra.org>.
On Thu, Jun 14, 2001 at 01:58:55AM -0700, Greg Stein wrote:
>...
> > 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.
I wanted to clarify my point a bit more here...
Documentation and a pool cleanup are ways to "paper over" (IMO) underlying
problems.
The problems can be avoided altogether by using another approach. (yes, doc
is still desired, but it isn't required to the same degree; the system is
robust even without doc)
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
Re: CVS update: MODIFIED: libsvn_subr ...
Posted by Greg Stein <gs...@lyra.org>.
On Wed, Jun 13, 2001 at 06:18:11PM -0500, Ben Collins-Sussman wrote:
> Greg Stein <gs...@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
Re: CVS update: MODIFIED: libsvn_subr ...
Posted by Ben Collins-Sussman <su...@collab.net>.
Greg Stein <gs...@lyra.org> writes:
> On Tue, Jun 12, 2001 at 08:48:58PM -0000, sussman@tigris.org wrote:
> > User: sussman
> > Date: 01/06/12 13:48:58
> >
> > Modified: subversion/libsvn_subr svn_error.c
> > Log:
> > * svn_error.c (initialize_svn_error_descriptions): new func - build a
> > hash table that maps svn errorcodes to english descriptions.
> >
> > (svn_strerror): take a pool instead of stringbuf. pass
> > this pool to initialize_svn_error_descriptions() the first time this
> > routine is called, and then cache the hash in a static variable
> > thereafter. use the hash for lookups.
> >
> > (svn_error_strings[]): removed, now part of hash initialization.
>
> 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. ;)
>
> 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.
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
>
> 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.
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.)
Then we came up with a solution we like better: instead of preventing
skew, make it harmless. We expect people to add a new svn errorcode
in svn_error.h, and then either forget to add a description in
svn_error.c, or perhaps add the descripiton in the wrong place. If
the descriptions were in a plain old array (as it used to be), then
either of these human errors would cause ALL the errors to be screwed
up, a big fat OBOE.
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.
> 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.
>
>
> 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. :-)
>
> [ 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.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org