xfs
[Top] [All Lists]

Re: [PATCH 1/2] Make stuff static

To: David Chinner <dgc@xxxxxxx>
Subject: Re: [PATCH 1/2] Make stuff static
From: Russell Cattelan <cattelan@xxxxxxxxxxx>
Date: Tue, 21 Nov 2006 19:09:43 -0600
Cc: Tim Shimmin <tes@xxxxxxx>, Eric Sandeen <sandeen@xxxxxxxxxxx>, xfs@xxxxxxxxxxx
In-reply-to: <20061122004216.GT11034@melbourne.sgi.com>
References: <20060929032856.8DA9C18001A5E@sandeen.net> <23F15D6AE8566A54B81188AC@timothy-shimmins-power-mac-g5.local> <45338DDE.8020903@sandeen.net> <4533FAEA.2080500@sandeen.net> <20061016232250.GM11034@melbourne.sgi.com> <1161042943.5723.117.camel@xenon.msp.redhat.com> <20061017005038.GN11034@melbourne.sgi.com> <AED98B89E193744D39BAC541@pmmelb207.melbourne.sgi.com> <20061017215706.GI8394166@melbourne.sgi.com> <1161125131.5723.158.camel@xenon.msp.redhat.com> <20061122004216.GT11034@melbourne.sgi.com>
Sender: xfs-bounce@xxxxxxxxxxx
On Wed, 2006-11-22 at 11:42 +1100, David Chinner wrote:
> On Tue, Oct 17, 2006 at 05:45:31PM -0500, Russell Cattelan wrote:
> > On Wed, 2006-10-18 at 07:57 +1000, David Chinner wrote:
> > > On Tue, Oct 17, 2006 at 05:13:01PM +1000, Tim Shimmin wrote:
> > > > I thought that for debug, we could stop them from being inline
> > > > for easier debugging. We could have a STATIC_INLINE :-)
> > > 
> > > We could, but I don't think it gains us anything.
> > I agree with Tim on this.
> > when I see STATIC in the code it's generally assumed to 
> > be a way to toggle of static on/off. Adding static inline 
> > to the #define STATIC starts to overload the the macro 
> > and creates an obfuscation that isn't immediately obvious.
> > STATIC_INLINE should be fairly obvious.
> 
> Ok, so I've had time to look at this again. Here's the definitions
> of STATIC and STATIC_INLINE for debug and nondebug from the
> patch (whitespace damaged):
> 
> Index: 2.6.x-xfs-new/fs/xfs/support/debug.h
> ===================================================================
> --- 2.6.x-xfs-new.orig/fs/xfs/support/debug.h   2006-11-22 10:54:37.089984780 
> +1100
> +++ 2.6.x-xfs-new/fs/xfs/support/debug.h        2006-11-22 11:30:20.433326839 
> +1100
> @@ -38,13 +38,37 @@ extern void assfail(char *expr, char *f,
> 
>  #ifndef DEBUG
>  # define ASSERT(expr)  ((void)0)
> -#else
> +
> +#ifndef STATIC
> +# define STATIC static noinline
> +#endif
> +
> +#ifndef STATIC_INLINE
> +# define STATIC_INLINE static inline
> +#endif
> +
> +#else /* DEBUG */
> +
>  # define ASSERT(expr)  ASSERT_ALWAYS(expr)
>  extern unsigned long random(void);
> -#endif
> 
>  #ifndef STATIC
> -# define STATIC static
> +# define STATIC noinline
> +#endif
> +
> +/*
> + * We stop inlining of inline functions in debug mode.
> + * Unfortunately, this means static inline in header files
> + * get multiple definitions, so they need to remain static.
> + * This then gives tonnes of warnings about unused but defined
> + * functions, so we need to add the unused attribute to prevent
> + * these spurious warnings.
> + */
> +#ifndef STATIC_INLINE
> +# define STATIC_INLINE static __attribute__ ((unused)) noinline
>  #endif
> 
> +#endif /* DEBUG */
> +
> +
>  #endif  /* __XFS_SUPPORT_DEBUG_H__ */
> 
> ------
> 
> Is this acceptible to everyone?
Yup.

> 
> FWIW, there is one other thing that this conversion causes
> problems with, and that's variable definitions. i.e. we can't
> use STATIC on them any more because of the "noinline" attribute
> it has. Do we care about this and if so, any suggestions on
> how to keep this functionality (a different STATIC_xxx define
> for structures)?
So I know things like systemtap kgdb oprofile all work better when
functions are not static, but what about variables/structures?
do things really get that confused?
Maybe we shouldn't worry about conditioning them and just make them
static

  


> 
> Cheers,
> 
> Dave.
-- 
Russell Cattelan <cattelan@xxxxxxxxxxx>

Attachment: signature.asc
Description: This is a digitally signed message part

<Prev in Thread] Current Thread [Next in Thread>