Re: [PATCH] --warnings=no-XXX [RFC] --warnings categories

[Akim Demaille]

Le 20 nov. 08 à 03:06, Di-an JAN a écrit :

Hi Di-an,

> This patch implemented -Wno-XXX as already documented in the manual,
> with -Wno-all = -Wnone (but without changing -Werror) and
> -Wno-none = -Wall,error (not worth fixing).
> As a side effect, no-XXX also works in --report and --trace.
>
> Two more problems: the manual says -Wno-syntax (not implemented)
> turns off ``useless'' warnings, and -Wnone only turn off -Wmidrule- 
> values
> not other warnings.  We need to categorize the existing warnings.
> RFC below.
>
> Patch tested with make check on Cygwin with one expected failure.

Please, install.

> @@ -102,11 +104,18 @@ flags_argmatch (const char *option,
>       args = strtok (args, ",");
>       while (args)
>         {
> -         int value = XARGMATCH (option, args, keys, values);
> +         int no = strncmp (args, "no-", 3) == 0 ? 3 : 0;
> +         int value = XARGMATCH (option, args + no, keys, values);
>           if (value == 0)

I'm surprised to see code like this in Bison.  Usually we use "!foo",  
not "foo == 0".


> -           *flags = 0;
> +           if (no)
> +             *flags = ~0;
> +           else
> +             *flags = 0;

And I much prefer ? : when it applies.  I can see there is value in  
symmetry here though.

>           else
> -           *flags |= value;
> +           if (no)
> +             *flags &= ~value;
> +           else
> +             *flags |= value;
>           args = strtok (NULL, ",");
>         }
>     }
>
> -----------------------------------------------------------------------
>
>         --warnings/-W categories
>
> -----------------------------------------------------------------------
>
> (basic): Things that don't work as intended.  Should be on by default.
> I don't have a good category name for these, but -Wnone would shut  
> them up
> (or not).
>
> "symbol `%s' used more than once as a literal string"
> "symbol `%s' given more than one literal string"
>  I think these should be errors, since the first is ambiguous
>  and the second we don't handle.

I agree and would make them errors.

> "conflicting outputs to file %s"

Should be error too.

> "%%expect-rr applies only to GLR parsers"
>  Why not allow this for non-GLR parsers?  Can people rely on the POSIX
>  rule that RR conflicts are resolved by choosing the earliest rule.

I don't know why this is GLR only, I would make it valid for all cases  
too.  Even though I dislike very much the current behavior where  
conflicts only trigger warnings.

> "%s affects only GLR parsers", "%dprec"
> "%s affects only GLR parsers", "%merge"

Could be left as warnings to make it easy for people to go from glr to  
deterministic parsing. Not sure it's common though :)

> "%s `%s' redefined", "%define variable"
> [%s `%s' is not used], "%define"/"%code"
>
> Might as well lump in other uncategorized warnings:
>
> "line number overflow"
> "column number overflow"

Yep.

> -----------------------------------------------------------------------
>
> -Wsyntax: Things that should not be problems but may be a matter of  
> style.
> Perhaps part of the above, turned off only by -Wnone.
> Should be on by default.
>
> "stray `$'"
> "stray `@'"
>  Probably shouldn't warn.  It should be up to the compiler.
>  In fact, Java code can have `@' (for annotations).

Agreed.

> "stray `,' treated as white space"

We have that???  Gee.  How about seeing how to get rid of it?

> "symbol %s redeclared"
>
> We can also warn about extra/missing semicolons and adjacent actions.

Makes sense.

> Perhaps also -Wyacc POSIX nonportabilities that can be worked around
> (e.g. %prec not at end of rule; not %locations).

Good with me.

> -----------------------------------------------------------------------
>
> -Wunused = -Wuseless
> Should be on by default.
>
> "nonterminal useless in grammar: %s"
> "rule useless in grammar"
> "rule useless in parser due to conflicts"
>
> There are actually two kinds.  Some things are ``useless'' because  
> they
> cannot derive a finite sequence of tokens such as:
>         start: | bad ;
>         bad: '*' bad ;
> I think these are always infinite loops and should be errors.
> The message should say something about that too.  It's easy to type
>         elements: elements | elements element ;
>
> Useful to turn off while developing a grammar so it's easier to see  
> other
> messages.

Maybe we're going too much into the details here.  I'm not sure we  
need many levels of warnings, we're not a compiler (yet :).  Another  
splitting method would be "grammar" for everything connected to the  
grammar, "action" or "semantics" for types and so forth.  Or split  
"semantics" and "code" (for missing ; for instance).

> -----------------------------------------------------------------------
>
> -Wtypes
> Should be on by default.
>
> "type clash on default action: <%s> != <%s>"
> "empty rule for typed nonterminal, and no action"
>
> Useful to turn off while writing semantic actions so it's easier to  
> see other
> messages.

Very much agreed.

> -----------------------------------------------------------------------
>
> -Wmidrule-values
>
> "unused value: $%d"
> "unset value: $$"
>
> -----------------------------------------------------------------------
>
> -Wyacc
>
> I'm working on that.

These make sense.  Before continuing, I'd appreciate input from others.

Di-an, could you please push in master the patch of yours that warns  
about the missing ; and adds them?  We can have it in 2.5 to drop  
the ; feature completely starting with 2.6 or so.