On Fri, 2008-06-13 at 18:33 +0200, Branko Èibej wrote:
> I disagree with this idea. Very very much in fact.
I didn't explain it well enough (or enough at all). Let me try harder.
> It's contrary to the
> API implementation rules we set up from day one --
I'm glad we have some rules. (I don't suppose you could give me a
pointer if they were written down?)
> one of which says that our API will not validate parameters.
Ah, now I think we may have a confusion between "the public APIs do not
guarantee to detect invalid parameters and report them to the caller",
which is fine and will remain the case, versus "no function, neither
public API nor internal, may attempt to check any of its parameters in
any way" which is clearly not the case.
I'm not particularly advocating any more or less parameter checking than
we have now. What I am advocating is freeing the library code from
in-built aborts without giving up the useful function pre-condition
checks that we have inserted in some places.
I intended this error-returning assertion as a replacement for uses of
the C "assert", so it will only be used where a programmer chose to put
in a check and where such a check would otherwise have aborted the
program.
The CHANGE from current behaviour to new behaviour when this macro is
substituted for current uses of assert() (in places where it can be
substituted without further changes) is:
* if a bug is encountered that triggers one of these assertions,
the program will now stop through Subversion's error reporting
mechanism instead of by aborting, except that:
* in cases where the software is designed to catch svn_error_t
error objects, it may now ignore the error or do something other
than the default of reporting the error and stopping.
> Also it would mean that all
> functions that use assert but happen not to return an svn_eror_t would
> have to be revved.
I didn't say we have to use this new macro absolutely everywhere from
day one. We could, for as long as we want to, leave any existing C
asserts in any API functions that don't return svn_error_t.
As and when we wish, we can "rev" them to return an error object. Where
we choose not to do so, the issue of having aborts in the library still
remains to be addressed by some separate thread of discussion if we
wish.
Of course, internal functions don't need to be revved even if we change
them to return an svn_error_t.
> The above implies that
>
> * assert should only be used for "this can't happen" sanity checks
Totally agree. That is, I agree irrespective of any particular
implementation or behaviour of "assert", because this is the very
definition of what I mean by the concept of "an assertion check" in a C
program. I would use some other terminology for other condition checks.
> * it should be turned off in production code (which is what we do on
> Windows, IIRC).
That's the original and common philosophy of "assert", but people have
strong and opposing arguments on this issue. I'm not addressing that
question in this thread and I would request that anyone wishing to
discuss it should please start a separate thread.
> And of course you /can/ catch an assert in a debugger.
Of course we can. If I said something that implied otherwise, it was
unintentional.
Oh, and direct "abort" calls: these are almost invariably used as a form
of assertion, and in those cases I advocate replacing them with
SVN_ERR_ASSERT.
No doubt there are still holes but I hope everyone has fewer concerns
now. Tell me what I've missed.
Regards,
- Juian
> -- Brane
>
> Julian Foad wrote:
> > PROBLEM
> > -------
> >
> > The standard C assertion, typically used at the beginning of a function
> > in statements like
> >
> > assert(arg1 != NULL);
> >
> > is an extremely useful self-checking tool for catching bugs, but in a C
> > library it has the unfortunate property that the program using the
> > library can't trap it. When that program is something big like an IDE or
> > the Windows desktop, the result of the ensuing crash is too destructive.
> >
> > Subversion's standard error reporting mechanism, which is mainly used
> > for reporting logical exceptions, can also be used to report bugs:
> >
> > if (! arg1)
> > return svn_error_create
> > (SVN_ERR_INCORRECT_PARAMS, NULL,
> > _("foo() cannot be called with a NULL arg1"));
> >
> > However, when written like this, it causes unnecessarily a decrease in
> > readability (4 lines just to indicate one simple fact) and an increase
> > in translator effort.
> >
> >
> > SOLUTION
> > --------
> >
> > A new macro:
> >
> > SVN_ERR_ASSERT(arg1 != NULL);
> >
> > which expands to something suitable, like the above code snippet,
> > returning an svn_error_t if the assertion fails.
> >
> >
> > Why does this seem too simple to be true?
> >
> > Any volunteers to do it?
> >
> > (Here's an improvement that can be made later: tweak the
> > "svn_error_clear" functions so that "assertion" errors can always be
> > discovered even if the programmer carelessly "clears" the error. Even
> > without this improvement, I think the new form will do a lot of good.)
> >
> > - 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-13 19:43:49 CEST