Re: prohibit_strcmp

[Akim Demaille]

Le 23 févr. 2012 à 17:52, Jim Meyering a écrit :

Hi Jim!

> Akim Demaille wrote:
>> So maybe this warning refers to another kind of STREQ/NEQ?  Such as:
>> 
>> fnmatch.c:# define STREQ(s1, s2) (strcmp (s1, s2) == 0)
>> fnmatch.c: (STREQ (string, "alpha") || STREQ (string, "upper") \
>> fnmatch.c: || STREQ (string, "lower") || STREQ (string, "digit") \
>> fnmatch.c: || STREQ (string, "alnum") || STREQ (string, "xdigit") \
>> fnmatch.c: || STREQ (string, "space") || STREQ (string, "print") \
>> fnmatch.c: || STREQ (string, "punct") || STREQ (string, "graph") \
>> fnmatch.c:    || STREQ (string, "cntrl") || STREQ (string, "blank"))
> 
> Those are in a file that's nominally maintained in glibc,
> so we cannot make such stylistic changes to it here in gnulib.

Ah, ok!

> That gnulib has two definitions is unfortunate, but I will not
> volunteer to change the STREQ that I've been using ;-)
> I don't see much value in streq.h, either, and hence, don't use it.

Any chance, if a patch was submitted to rename the other one,
that it'd be accepted?  What's the policy wrt breaking APIs?

> In the same vein, it's good to avoid strncmp calls.
> When comparing against a literal (and with no trailing NULL),
> I use STRNCMP_LIT defined in coreutils/src/system.h:
> 
>    /* Just like strncmp, but the second argument must be a literal string
>       and you don't specify the length.  */
>    #define STRNCMP_LIT(s, literal) \
>      strncmp (s, "" literal "", sizeof (literal) - 1)

Good to know, thanks!

>> which, I concur, is more readable.  It is confusing that there are
>> two sets of STREQ in gnulib, and at least the comment for prohibit_strcmp
>> should point this out.
> 
> The definition I intended to suggest via that syntax-check rule is
> short enough that we could even include it in the diagnostic, I suppose.
> 
>  #define STREQ(a, b) (strcmp (a, b) == 0)

OK, that's what I did for Bison, thanks!

> How about this?
> 
> diff --git a/top/maint.mk b/top/maint.mk
> index 1dd6493..2841773 100644
> --- a/top/maint.mk
> +++ b/top/maint.mk
> @@ -303,8 +303,10 @@ sc_prohibit_atoi_atof:
>         $(_sc_search_regexp)
> 
> # Use STREQ rather than comparing strcmp == 0, or != 0.
> +s_ = str''cmp
> +sp_ = $(s_) *\(.+\)
> sc_prohibit_strcmp:
> -     @grep -nE '! *str''cmp *\(|\<str''cmp *\(.+\) *[!=]='   \
> +     @grep -nE '! *$(s_) *\(|\<$(sp_) *[!=]=|[!=]= *$(sp_)'          \
>           $$($(VC_LIST_EXCEPT))                                       \
>         | grep -vE ':# *define STRN?EQ\(' &&                          \
>         { echo '$(ME): replace str''cmp calls above with STREQ/STRNEQ' \

Success:

prohibit_strcmp
../../doc/bison.texinfo:2572:    if (strcmp (ptr->name,sym_name) == 0)
../../src/files.c:145:  if (strcmp (ext, ".y") == 0)
../../src/files.c:350:  if (0 == strcmp (*file_name, grammar_file))
../../src/files.c:360:        if (0 == strcmp (file_names[i], *file_name))
../../src/ielr.c:1098:    if (0 == strcmp (type, "lalr"))
../../src/ielr.c:1100:    else if (0 == strcmp (type, "ielr"))
../../src/ielr.c:1102:    else if (0 == strcmp (type, "canonical-lr"))
../../src/lalr.c:376:      0 == strcmp (default_reductions, "accepting");
../../src/muscle-tab.c:53:  return strcmp (m1->key, m2->key) == 0;
../../src/muscle-tab.c:410:    if (!strcmp (conversion[i].obsolete, variable))
../../src/muscle-tab.c:558:      if (value[0] == '\0' || 0 == strcmp (value, 
"true"))
../../src/muscle-tab.c:560:      else if (0 == strcmp (value, "false"))
../../src/muscle-tab.c:617:              if (0 == strcmp (value, *values))
../../src/output.c:692:      && 0 != strcmp (use_push_for_pull_env, "0"))
../../src/print.c:339:      aver (0 == strcmp (default_reductions, "most")
../../src/print.c:340:            || (0 == strcmp (default_reductions, 
"consistent")
../../src/print_graph.c:136:            && strcmp (symbols[sym]->tag, "error") 
!= 0)
../../src/reader.c:641:    if (0 != strcmp (lr_type, "canonical-lr"))
../../src/scan-skel.l:182:  if (0 == strcmp (at_directive_argv[0], "@basename"))
../../src/scan-skel.l:188:  else if (0 == strcmp (at_directive_argv[0], "@warn")
../../src/scan-skel.l:189:           || 0 == strcmp (at_directive_argv[0], 
"@complain")
../../src/scan-skel.l:190:           || 0 == strcmp (at_directive_argv[0], 
"@fatal"))
../../src/scan-skel.l:226:  else if (0 == strcmp (at_directive_argv[0], 
"@warn_at")
../../src/scan-skel.l:227:           || 0 == strcmp (at_directive_argv[0], 
"@complain_at")
../../src/scan-skel.l:228:           || 0 == strcmp (at_directive_argv[0], 
"@fatal_at"))
../../src/scan-skel.l:269:  else if (0 == strcmp (at_directive_argv[0], 
"@output"))
../../src/tables.c:312:    if (0 != strcmp (default_reductions, "most") && 
!s->consistent)
../../src/uniqstr.c:114:  return strcmp (m1, m2) == 0;
maint.mk: replace strcmp calls above with STREQ/STRNEQ
gmake: *** [sc_prohibit_strcmp] Error 1