On Tue, 2008-06-17 at 13:56 +0200, Vincent Lefevre wrote:
> On 2008-06-17 10:47:10 +0100, Julian Foad wrote:
> > I think assertions are most often triggered by simple bugs that have no
> > risk of writing corrupt data to the repository or to anywhere, where the
> > code merely encounters some unexpected combination of inputs that are
> > individually valid. In these common cases, there is no danger in
> > allowing the program to continue (either just to do a bit of cleaning up
> > before exiting, or to keep going with other operations).
>
> OK, but then, wouldn't it be better to have different kinds of
> assertions? (Perhaps this is a part of what you mention below.)
> Or is it too difficult to know in advance whether some failed
> assertion is really bad?
What different kinds?
It's impossible to know in advance whether some unexpected condition
(leading to a failed assertion) is "really bad". Assertions are to catch
conditions that "can't happen". If you tried to analyse the code to find
how one of those conditions could happen, you would either find nothing
or you would find a bug and fix it.
> > Remember that the conditions checked by assertion statements are not
> > very different from, and sometimes exactly the same as, conditions that
> > we already check with normal validation checks:
> >
> > /** ... and @a rev should be of kind "head" or "base", otherwise
> > * we'll throw an error. */
> > svn_func1(svn_opt_revision_t rev)
> > {
> > /* This API is designed such that it's our responsibility
> > to validate this argument. */
> > if (rev->kind != svn_opt_revision_head
> > && rev->kind != svn_opt_revision_base)
> > return error(SVN_ERR_INVALID_REVISION_KIND);
> > ...
> > }
> >
> > /** ... and @a rev must be of kind "head" or "base". */
> > svn_func2(svn_opt_revision_t rev)
> > {
> > /* This API is designed with API requirements that the
> > * caller must obey. To assist development and debugging
> > * we enforce the requirements with an assertion. */
> > SVN_ERR_ASSERT(rev->kind == svn_opt_revision_head
> > || rev->kind == svn_opt_revision_base);
> > ...
> > }
>
> I think an error reported to the caller is generally quite safe here
> (as long as it is handled correctly) because the check is done early
> enough. But if you have started to modify internal structures then
> do a consistency check and find that something is wrong, then I'd
> say that an immediate abort would be better than trying to clean up,
> because the clean up could be based on wrong data.
No. You're thinking there is only one level of function call involved.
In reality an assertion is likely to fail in a deeply nested function
after the higher level functions have set up lots of state. However,
most of this state is local to the functions involved, so it's still
pretty safe to clean up a few important things like a connection to the
repository, and ignore all the rest of the state that was set up (mostly
in pools and local variables).
I'll say again: this is hardly any different from any other
error-returning situation in many respects. Where the programmer is
aware that returning out of the middle of a function could leave an
undesired incomplete state, he saves the error return, does the
necessary clean-up, and then returns the error:
set_up();
err = do_other_stuff_that_might_go_wrong();
clean_up();
return err;
There's no problem here.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-06-17 14:12:43 CEST