[asterisk-dev] [svn-commits] tilghman: trunk r116694 - in /trunk/include/asterisk: strings.h utils.h

[Tilghman Lesher]

On Thursday 15 May 2008 18:49:07 Tilghman Lesher wrote:
> On Thursday 15 May 2008 18:18:43 Russell Bryant wrote:
> > SVN commits to the Digium repositories wrote:
> > > Author: tilghman
> > > Date: Thu May 15 17:05:47 2008
> > > New Revision: 116694
> > >
> > > URL: http://svn.digium.com/view/asterisk?view=rev&rev=116694
> > > Log:
> > > Add an extra check in ast_strlen_zero, and make ast_assert() not print
> > > the file, line, and function name twice.
> > > (Closes issue #12650)
> > >
> > > Modified:
> > >     trunk/include/asterisk/strings.h
> > >     trunk/include/asterisk/utils.h
> > >
> > > Modified: trunk/include/asterisk/strings.h
> > > URL:
> > > http://svn.digium.com/view/asterisk/trunk/include/asterisk/strings.h?vi
> > >ew =diff&rev=116694&r1=116693&r2=116694
> > > =======================================================================
> > >== ===== --- trunk/include/asterisk/strings.h (original)
> > > +++ trunk/include/asterisk/strings.h Thu May 15 17:05:47 2008
> > > @@ -29,10 +29,25 @@
> > >
> > >  /* You may see casts in this header that may seem useless but they
> > > ensure this file is C++ clean */
> > >
> > > +#ifdef AST_DEVMODE
> > > +#define ast_strlen_zero(foo)	_ast_strlen_zero(foo, __FILE__,
> > > __PRETTY_FUNCTION__, __LINE__) +static force_inline int
> > > _ast_strlen_zero(const char *s, const char *file, const char *function,
> > > int line) +{
> > > +	if (!s || (*s == '\0')) {
> > > +		return 1;
> > > +	}
> > > +	if (!strcmp(s, "(null)")) {
> > > +		ast_log(__LOG_WARNING, file, line, function, "Possible programming
> > > error: \"(null)\" is not NULL!\n"); +	}
> > > +	return 0;
> > > +}
> > > +
> > > +#else
> > >  static force_inline int ast_strlen_zero(const char *s)
> > >  {
> > >  	return (!s || (*s == '\0'));
> > >  }
> > > +#endif
> >
> > The entire reason that I added ast_assert(), posted the patch to this
> > issue, and referenced this issue in the commit, is because I was
> > proposing that ast_assert() should be used for this check, instead of all
> > of this work to conditionally define this function, and every other
> > function that you want to check arguments to for debugging purposes.
> >
> > I would like to see this change redone and simplified to use
> > ast_assert().
>
> If only that actually worked the way you hoped that it would:
>
> FRACK!, Failed assertion !strcmp(s, "(null)") (0) at line 40 in
> _ast_strlen_zero
> of /home/tilghman/Asterisk/asterisk-trunk/include/asterisk/strings.h
> [May 15 18:43:12]
> ERROR[26580]:
> /home/tilghman/Asterisk/asterisk-trunk/include/asterisk/strings.h40
> _ast_strlen_zero: FRACK!, Failed assertion !strcmp(s, "(null)") (0)
>
> So instead of telling me WHERE it failed in the code, it tells me that it
> failed in include/asterisk/strings.h, which isn't very helpful.  If you
> could show me how to make the error message useful, I'll change it.
>
> Oh, and in case you're wondering, I added the opposite check in the code,
> because I hadn't gotten the message to show up in my tests.  So the
> operation shown is actually the wrong one, just to show that the wrong
> message got printed.

The closest I can make it to using the assertion code is to use the underlying
private function:

Index: include/asterisk/strings.h
===================================================================
--- include/asterisk/strings.h  (revision 116694)
+++ include/asterisk/strings.h  (working copy)
@@ -36,9 +36,7 @@
        if (!s || (*s == '\0')) {
                return 1;
        }
-       if (!strcmp(s, "(null)")) {
-               ast_log(__LOG_WARNING, file, line, function, "Possible 
programming error: \"(null)\" is not NULL!\n");
-       }
+       _ast_assert(strcmp(s, "(null)"), "strcmp(s, \"(null)\")", file, line, 
function);
        return 0;
 }

which is probably still not what you had intended.

-- 
Tilghman