[PATCH] clean up some debugging code

[Stefan Sperling <st...@elego.de>]

Hi,

comments, anyone?

I'd like to commit this, but have one open question (read the log
message first, my question is below).

The patch passes 'make check'.

[[[

Remove assertions and other debugging code ifdef'd with
NDEBUG if debugging is not enabled at compile time.
So far, this code was always compiled in, even in releases.

In path handling functions, assert that paths supplied
are canonical when debugging is enabled. This isn't new,
some functions already did so before this change, but now
it is consistent, and together with the above change,
doing this now actually makes sense.

* subversion/libsvn_subr/path.c
  (is_canonical): Document, extend a little, and rewrite partly
   for obviousness. This function has been around for much longer
   than svn_path_is_canonical, and tries to provide the same
   functionality, but does not quite match up. Confine its use
   to functions that don't have a pool around.
  (svn_path_join, svn_path_join_many, svn_path_add_component,
   svn_path_remove_component, svn_path_dirname, svn_path_basename,
   svn_path_is_empty, svn_path_compare_paths, svn_path_is_child,
   svn_path_decompose, svn_path_is_single_path_component,
   svn_path_split_if_file): When debugging is enabled, assert that
    paths supplied are canonical, using svn_path_is_canonical if possible,
    otherwise using is_canonical. The API requires all paths supplied to
    be canonical. Some of these functions have already done the same
    assertion with is_canonical before, but for a few the assertion was
    commented out because the call to strlen involved was considered
    expensive ...eh? This is debugging code, it's not supposed to
    perform well, it's a safety net.

* subversion/tests/libsvn_subr/path-test.c
  (test_is_single_path_component): Don't use a non-canonical path
   in test set, the API being tested here does not support this.
   This was only working because the assertion in the function
   being tested was commented out.

* configure.ac: Compile with -DNDEBUG if debugging is off.

]]]

The Open Question:

The -DNDEBUG flag only shows up when I run configure with
--disable-debug. It looks like the default for --enable-debug is "yes"
or simply undefined (according to Daniel Shahaf, I still have to verify
this). But shouldn't we rather default to --enable-debug="no"?

Thanks,
Stefan

-- 
This .sig provided for the sanity of danielsh

Re: [PATCH] clean up some debugging code

[Stefan Sperling <st...@elego.de>]

On Thu, May 08, 2008 at 10:26:24AM +0200, Stefan Sperling wrote:
> On Thu, May 08, 2008 at 10:19:34AM +0200, Stefan Sperling wrote:
> > Hi,
> > 
> > comments, anyone?
> > 
> > I'd like to commit this, but have one open question (read the log
> > message first, my question is below).
> > 
> > The patch passes 'make check'.
> 
> > @@ -347,7 +367,7 @@ svn_path_add_component(svn_stringbuf_t *path,
> >  void
> >  svn_path_remove_component(svn_stringbuf_t *path)
> >  {
> > -  assert(is_canonical(path->data, path->len));
> > +  assert(is_canonical(path->data, strlen(path->data)));
> 
> Mmmmh. There's a few redundant strlen calls in it now alright,
> they happened because I did a global search/replace edit.
> 
> I will fix those, i.e. use ->len again where possible, before commit.

No comments means consent, right? :)

(Well, actually Daniel said the patch was OK in IRC.)

Committed in r31121, forgot to fix it to use ->len though, that's
done in r31122.

The default for --enable-debug is still an open question, but can be
addressed separately.

Stefan

Re: [PATCH] clean up some debugging code

[Stefan Sperling <st...@elego.de>]

On Thu, May 08, 2008 at 10:19:34AM +0200, Stefan Sperling wrote:
> Hi,
> 
> comments, anyone?
> 
> I'd like to commit this, but have one open question (read the log
> message first, my question is below).
> 
> The patch passes 'make check'.

> @@ -347,7 +367,7 @@ svn_path_add_component(svn_stringbuf_t *path,
>  void
>  svn_path_remove_component(svn_stringbuf_t *path)
>  {
> -  assert(is_canonical(path->data, path->len));
> +  assert(is_canonical(path->data, strlen(path->data)));

Mmmmh. There's a few redundant strlen calls in it now alright,
they happened because I did a global search/replace edit.

I will fix those, i.e. use ->len again where possible, before commit.

Stefan