[spice-server,05/10] syntax-check: Check ENABLE_EXTRA_CHECKS is not used incorrectly

Submitted by Frediano Ziglio on Sept. 11, 2017, 10:15 a.m.

Details

Message ID 20170911101547.19433-6-fziglio@redhat.com
State New
Headers show
Series "Miscellaneous patches" ( rev: 13 12 11 ) in Spice

Not browsing as part of any series.

Commit Message

Frediano Ziglio Sept. 11, 2017, 10:15 a.m.
Usually configuration macros are defined to 0 or undefined.
For this reason these macros are sometimes checked using #if
and sometimes with #ifndef/#ifdef.
As this macro is always defined with 0 or 1 it makes no sense
to check if defined or not so check the code to avoid this
mistake.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
---
 cfg.mk | 8 ++++++++
 1 file changed, 8 insertions(+)

Patch hide | download patch | download mbox

diff --git a/cfg.mk b/cfg.mk
index 93d7040c5..6bd3c55b3 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -111,6 +111,14 @@  sc_copyright_format:
 	halt='spell Red Hat as two words'				\
 	  $(_sc_search_regexp)
 
+# ENABLE_EXTRA_CHECKS is always defined, do not allow
+# "#ifndef ENABLE_EXTRA_CHECKS"
+sc_extra_checks:
+	@prohibit='#[[:space:]]*ifn?def[[:space:]]+ENABLE_EXTRA_CHECKS'	\
+	in_vc_files='\.[ch]$$'						\
+	halt='ENABLE_EXTRA_CHECKS is always defined'			\
+	  $(_sc_search_regexp)
+
 # We don't use this feature of maint.mk.
 prev_version_file = /dev/null
 

Comments

On Mon, Sep 11, 2017 at 11:15:42AM +0100, Frediano Ziglio wrote:
> Usually configuration macros are defined to 0 or undefined.
> For this reason these macros are sometimes checked using #if
> and sometimes with #ifndef/#ifdef.
> As this macro is always defined with 0 or 1 it makes no sense
> to check if defined or not so check the code to avoid this
> mistake.

IIRC, I suggested a way not to make it so odd compared to other
preprocessor symbols.

Christophe

> 
> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> ---
>  cfg.mk | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/cfg.mk b/cfg.mk
> index 93d7040c5..6bd3c55b3 100644
> --- a/cfg.mk
> +++ b/cfg.mk
> @@ -111,6 +111,14 @@ sc_copyright_format:
>  	halt='spell Red Hat as two words'				\
>  	  $(_sc_search_regexp)
>  
> +# ENABLE_EXTRA_CHECKS is always defined, do not allow
> +# "#ifndef ENABLE_EXTRA_CHECKS"
> +sc_extra_checks:
> +	@prohibit='#[[:space:]]*ifn?def[[:space:]]+ENABLE_EXTRA_CHECKS'	\
> +	in_vc_files='\.[ch]$$'						\
> +	halt='ENABLE_EXTRA_CHECKS is always defined'			\
> +	  $(_sc_search_regexp)
> +
>  # We don't use this feature of maint.mk.
>  prev_version_file = /dev/null
>  
> -- 
> 2.13.5
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
> On Mon, Sep 11, 2017 at 11:15:42AM +0100, Frediano Ziglio wrote:
> > Usually configuration macros are defined to 0 or undefined.
> > For this reason these macros are sometimes checked using #if
> > and sometimes with #ifndef/#ifdef.
> > As this macro is always defined with 0 or 1 it makes no sense
> > to check if defined or not so check the code to avoid this
> > mistake.
> 
> IIRC, I suggested a way not to make it so odd compared to other
> preprocessor symbols.
> 
> Christophe
>

Yes, but your suggestion did not work and you didn't send another
improvement (unless I lost it).

Frediano
 
> > 
> > Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> > ---
> >  cfg.mk | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/cfg.mk b/cfg.mk
> > index 93d7040c5..6bd3c55b3 100644
> > --- a/cfg.mk
> > +++ b/cfg.mk
> > @@ -111,6 +111,14 @@ sc_copyright_format:
> >  	halt='spell Red Hat as two words'				\
> >  	  $(_sc_search_regexp)
> >  
> > +# ENABLE_EXTRA_CHECKS is always defined, do not allow
> > +# "#ifndef ENABLE_EXTRA_CHECKS"
> > +sc_extra_checks:
> > +	@prohibit='#[[:space:]]*ifn?def[[:space:]]+ENABLE_EXTRA_CHECKS'	\
> > +	in_vc_files='\.[ch]$$'						\
> > +	halt='ENABLE_EXTRA_CHECKS is always defined'			\
> > +	  $(_sc_search_regexp)
> > +
> >  # We don't use this feature of maint.mk.
> >  prev_version_file = /dev/null
> >
On Mon, Sep 11, 2017 at 11:48:47AM -0400, Frediano Ziglio wrote:
> >
> > On Mon, Sep 11, 2017 at 11:15:42AM +0100, Frediano Ziglio wrote:
> > > Usually configuration macros are defined to 0 or undefined.
> > > For this reason these macros are sometimes checked using #if
> > > and sometimes with #ifndef/#ifdef.
> > > As this macro is always defined with 0 or 1 it makes no sense
> > > to check if defined or not so check the code to avoid this
> > > mistake.
> > 
> > IIRC, I suggested a way not to make it so odd compared to other
> > preprocessor symbols.
> > 
> > Christophe
> >
> 
> Yes, but your suggestion did not work and you didn't send another
> improvement (unless I lost it).

Ah, sorry, I did not answer because it seemed easy enough to adjust
it to something which works, iirc the issue was that there was a
non-compile-time constant:
static const int foo = ENABLE_EXTRA_CHECKS;
I guess this should work instead:
#define foo ENABLE_EXTRA_CHECKS

Christophe
> 
> On Mon, Sep 11, 2017 at 11:48:47AM -0400, Frediano Ziglio wrote:
> > >
> > > On Mon, Sep 11, 2017 at 11:15:42AM +0100, Frediano Ziglio wrote:
> > > > Usually configuration macros are defined to 0 or undefined.
> > > > For this reason these macros are sometimes checked using #if
> > > > and sometimes with #ifndef/#ifdef.
> > > > As this macro is always defined with 0 or 1 it makes no sense
> > > > to check if defined or not so check the code to avoid this
> > > > mistake.
> > > 
> > > IIRC, I suggested a way not to make it so odd compared to other
> > > preprocessor symbols.
> > > 
> > > Christophe
> > >
> > 
> > Yes, but your suggestion did not work and you didn't send another
> > improvement (unless I lost it).
> 
> Ah, sorry, I did not answer because it seemed easy enough to adjust
> it to something which works, iirc the issue was that there was a
> non-compile-time constant:
> static const int foo = ENABLE_EXTRA_CHECKS;
> I guess this should work instead:
> #define foo ENABLE_EXTRA_CHECKS
> 
> Christophe
> 

Mumble, maybe I don't understand your suggestion here.
The odd thing is that ENABLE_EXTRA_CHECKS is always defined.
If we move to the "standard" 1 or undefined your code like

   if (foo) ...

will get (defined to 1):

   if (1) ...

and compile, but will get (undefined):

   if (ENABLE_EXTRA_CHECKS) ...

and compiled will complain that ENABLE_EXTRA_CHECKS is an undefined id.

Frediano