[sheepdog] [PATCH] move assert definition from util.h to logger.h

[Liu Yuan]

On Thu, Jul 11, 2013 at 05:13:38PM +0900, MORITA Kazutaka wrote:
> At Thu, 11 Jul 2013 15:45:05 +0800,
> Liu Yuan wrote:
> > 
> > On Thu, Jul 11, 2013 at 04:31:44PM +0900, MORITA Kazutaka wrote:
> > > At Thu, 11 Jul 2013 12:55:18 +0800,
> > > Liu Yuan wrote:
> > > > 
> > > > On Thu, Jul 11, 2013 at 01:11:57PM +0900, MORITA Kazutaka wrote:
> > > > > If we don't use a logger, we don't need to define our assert.  In that
> > > > > sense, assert() should be defined in logger.h.  The only problem is
> > > > > that assert can be used before including logger.h.  To avoid it, this
> > > > > patch adds an #error directive to detect assert definition before
> > > > > logger.h.
> > > > > 
> > > > > This fixes a compiler error which happens when NDEBUG is not defined.
> > > > > 
> > > > > Signed-off-by: MORITA Kazutaka <morita.kazutaka at lab.ntt.co.jp>
> > > > > ---
> > > > >  include/compiler.h       |  3 +++
> > > > >  include/logger.h         | 14 +++++++++-----
> > > > >  include/sheep.h          |  2 +-
> > > > >  include/sheepdog_proto.h |  3 ++-
> > > > >  include/util.h           | 24 +++++-------------------
> > > > >  lib/event.c              |  2 +-
> > > > >  lib/net.c                |  2 +-
> > > > >  lib/util.c               |  3 +--
> > > > >  lib/work.c               |  2 +-
> > > > >  sheep/sheep_priv.h       |  1 +
> > > > >  sheepfs/volume.c         |  1 -
> > > > >  shepherd/shepherd.c      |  1 -
> > > > >  12 files changed, 25 insertions(+), 33 deletions(-)
> > > > > 
> > > > > diff --git a/include/compiler.h b/include/compiler.h
> > > > > index 1cd2440..b2ae002 100644
> > > > > --- a/include/compiler.h
> > > > > +++ b/include/compiler.h
> > > > > @@ -17,4 +17,7 @@
> > > > >  
> > > > >  #define __printf(a, b) __attribute__((format(printf, a, b)))
> > > > >  
> > > > > +/* Force a compilation error if the condition is true */
> > > > > +#define BUILD_BUG_ON(condition) ((void)sizeof(struct { int: -!!(condition); }))
> > > > > +
> > > > >  #endif	/* SD_COMPILER_H */
> > > > > diff --git a/include/logger.h b/include/logger.h
> > > > > index c2c4237..9bc3bbe 100644
> > > > > --- a/include/logger.h
> > > > > +++ b/include/logger.h
> > > > > @@ -81,10 +81,14 @@ void sd_backtrace(void);
> > > > >  		log_write(SDOG_DEBUG, __func__, __LINE__, fmt, ##args);	\
> > > > >  	} while (0)
> > > > >  
> > > > > -static inline void do_assert(const char *expr, const char *func, int line)
> > > > > -{
> > > > > -	log_write(SDOG_EMERG, func, line, "Asserting `%s' failed.", expr);
> > > > > -	abort();
> > > > > -}
> > > > > +#ifdef assert
> > > > > +#error "logger.h must be included before assert.h"
> > > > 
> > > > I think we should always use sheepdog's assert(). So this would be better rephrased
> > > > as "Don't include <assert.h>, use logger.h for assert()"
> > > 
> > > I thought that at least sheepfs doesn't want to include logger.h.  Is
> > > it wrong?  Our sd_printf() and assert() print a thread name and it is
> > > meaningless for the programs who don't use worker threads.
> > > 
> > 
> > I don't see the conflict. This error only is fired when people include <assert.h>
> > and logger.h in the same source file. sheepfs doesn't include logger.h.
> > 
> > I don't see the point both include assert.h and logger.h in the same source file
> 
> Okay, so your message
>   "Don't include <assert.h>, use logger.h for assert()"
> is for the users who include both assert.h and logger.h, right?  The
> user already includes logger.h, so the message "use logger.h" looks a
> bit strange.
> 
> "Don't include assert.h and logger.h in the same source file." looks
> straightforward and don't mislead the developer who don't want to use
> logger.h.

Okay, but I think the main purpose of this message is stop users from including
<assert.h>, no? There is not use case to include <assert.h> in sheep/collie/shepherd.

Thanks
Yuan