Re: [patch] Avoid some of the sc_ rules to freeze

[Eric Blake]

According to address@hidden on 2/25/2010 4:26 PM:
> I heavily expanded the _prohibit_regexp macro to accept more arguments
> and renamed it to _sc_search_regexp.  Using the new parameters it is
> possible to write rules like:
> 
> # Ensure that each .c file containing a "main" function also
> # calls set_program_name.
> sc_program_name:
>       @require='set_program_name *\(m?argv\[0\]\);'                   \
>       in_files='\.c$$'                                                \
>       containing='^main *('                                           \
>       msg='the above files do not call set_program_name'              \
>       $(_sc_search_regexp)

Slick.

> +# _sc_search_regexp
> +#
> +# This macro searches for a given construct in the selected files and
> +# then takes some action.
> +#
> +# Parameters (environment variables):

Shell variables, actually; we don't require that they be exported to the
environment.

> +#
> +#  prohibit | require
> +#
> +#     Regular expression denoting either a forbidded construct or a

s/forbidded/forbidden/.  It would be worth clarifying BRE or ERE.

> +#     required construct.  Those arguments are exclusive.
> +#
> +#  in_files | not_in_files
> +#
> +#     grep-E-style regexp denoting the files to check.
> +#
> +#  containing | non_containing
> +#
> +#     Select the files (non) containing strings matching this regexp.
> +#     If both arguments are specified then CONTAINING takes
> +#     precedence.
> +#
> +#  with_grep_options
> +#
> +#     Extra options for grep.
> +#
> +#  ignore_case
> +#
> +#     Ignore case.
> +#
> +#  halt | warn
> +#
> +#     Message to display before to halt the execution.

s/halt the execution/halting execution or warning/

> +
> +# By default, _sc_search_regexp does not ignore case.
>  export ignore_case =
>  _ignore_case = $$(test -n "$$ignore_case" && echo -i || :)
>  
> -# There are many rules below that prohibit constructs in this package.
> -# If the offending construct can be matched with a grep-E-style regexp,
> -# use this macro.  The shell variables "re" and "msg" must be defined.
> -define _prohibit_regexp
> +define _sc_say_and_exit
> +   dummy=; : so we do not need a semicolon before each use;                  
>         \
> +   { echo "$(ME): $$msg" 1>&2; exit 1; };
> +endef
> +
> +define _sc_search_regexp
> +   dummy=; : so we do not need a semicolon before each use;                  
>         \
> +                                                                             
>         \
> +   : Check arguments;                                                        
>                 \

Line up the \.  (I wonder if my MUA will botch line endings when sending
this reply).

> +   test -n "$$prohibit" -a -n "$$require" &&                                 
>         \

'test -a' is not portable, and POSIX 2008 recommends against its use.
It's not that hard to get in the habit of using 'test && test' instead.

> +     { msg='Cannot specify both prohibit and require' $(_sc_say_and_exit) } 
> || :;    \
> +   test -z "$$prohibit" -a -z "$$require" &&                                 
>         \
> +     { msg='Should specify either prohibit or require' $(_sc_say_and_exit) } 
> || :;   \
> +   test -n "$$in_files" -a -n "$$not_in_files" &&                            
>         \
> +     { msg='Cannot specify both in-files and not_in_files'                   
>         \
> +             $(_sc_say_and_exit) } || :;                                     
>         \
> +   test "x$$msg" != x || { msg='msg not defined' $(_sc_say_and_exit) };      
>                 \
> +                                                                             
>         \
> +   : Filter by file name;                                                    
>         \
> +   if test -n "$$in_files"; then                                             
>         \
> +     files=$$($(VC_LIST_EXCEPT) | grep -E "$$in_files");                     
>         \
> +   else                                                                      
>                 \
> +     files=$$($(VC_LIST_EXCEPT));                                            
>         \
> +   fi;                                                                       
>                 \
> +                                                                             
>         \
> +   : Filter by content;                                                      
>                 \
> +   test -n "$$files" -a -n "$$containing" &&                                 
>         \
> +     { files=$$(grep -l "$$containing" $$files); } || :;                     
>         \
> +   test -n "$$files" -a -n "$$non_containing" &&                             
>         \
> +     { files=$$(grep -vl "$$non_containing" $$files); } || :;                
>         \
> +                                                                             
>         \
> +   : Check for the construct;                                                
>                 \
> +   if test -n "$$files"; then                                                
>                 \
> +      if test -n "$$prohibit"; then                                          
>         \
> +     grep $$with_grep_options $(_ignore_case) -nE "$$prohibit" $$files &&    
>         \
> +       { msg="$$msg" $(_sc_say_and_exit) } || :;                             
>         \
> +      else                                                                   
>         \
> +     grep $$with_grep_options $(_ignore_case) -LE "$$require" $$files | grep 
> . &&    \
> +       { msg="$$msg" $(_sc_say_and_exit) } || :;                             
>         \
> +      fi                                                                     
>         \
> +   else :;                                                                   
>         \
> +  fi || :;
> +endef
> +
> +# To use this "command" macro, you must first define two shell variables:
> +# h: the header, enclosed in <> or ""
> +# re: a regular expression that matches IFF something provided by $h is used.
> +define _sc_header_without_use
>    dummy=; : so we do not need a semicolon before each use;           \
> -  test "x$$re" != x || { echo '$(ME): re not defined' 1>&2; exit 1; };       
> \
> -  test "x$$msg" != x || { echo '$(ME): msg not defined' 1>&2; exit 1; };\
> -  grep $(_ignore_case) -nE "$$re" $$($(VC_LIST_EXCEPT)) &&           \
> -    { echo '$(ME): '"$$msg" 1>&2; exit 1; } || :
> +  h_esc=`echo "$$h"|sed 's/\./\\\\./g'`;                             \
> +  if $(VC_LIST_EXCEPT) | grep -l '\.c$$' > /dev/null; then           \
> +    files=$$(grep -l '^# *include '"$$h_esc"                         \
> +          $$($(VC_LIST_EXCEPT) | grep '\.c$$')) &&                   \
> +    grep -LE "$$re" $$files | grep . &&                                      
> \
> +      { echo "$(ME): the above files include $$h but don't use it"   \
> +     1>&2; exit 1; } || :;                                           \
> +  else :;                                                            \
> +  fi
>  endef

Lots of decent looking changes in the rest of the file, but I didn't look
closely.  I did, however, spot some things that might warrant a resubmission:

>  
>  sc_avoid_if_before_free:
>  # Error messages should not end with a period
>  sc_error_message_period:
> -     @grep -nEA2 '[^rp]error \(' $$($(VC_LIST_EXCEPT))               \
> +     @grep -nEA2 '[^rp]error *\(' $$($(VC_LIST_EXCEPT))              \

Changes like this are worth an independent patch.  'git add -i' can be
helpful in splitting a patch.

>  sc_prohibit_openat_without_use:
>       @h='"openat.h"' \
>       
> re='\<(openat_(permissive|needs_fchdir|(save|restore)_fail)|l?(stat|ch(own|mod))at|(euid)?accessat)\>'
>  \
> -       $(_header_without_use)
> +       $(_sc_header_without_use)

For that matter, changes like this could be done in stages for easier
reviewing; one patch to rename _{,sc_}header_without_use, and another one
to redefine _sc_header_without_use in terms of your new powerhouse macro.

-- 
Eric Blake   address@hidden    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature