Re: [C++ PATCH] New warning for extra semicolons after in-class function definitions

[David Malcolm <dmalcolm@redhat.com>]

On Fri, 2017-04-07 at 21:55 +0200, Volker Reichelt wrote:
> Hi,
> 
> with the following patch I suggest to add a diagnostic for extra
> semicolons after in-class member function definitions:
> 
>   struct A
>   {
>     A() {};
>     void foo() {};
>     friend void bar() {};
>   };
> 
> Although they are allowed in the C++ standard, people (including me)
> often like to get rid of them for stylistic/consistency reasons.
> In fact clang has a warning -Wextra-semi for this.
> 
> Also in GCC (almost exactly 10 years ago) there was a patch
> https://gcc.gnu.org/ml/gcc-cvs/2007-03/msg00841.html
> to issue a pedwarn (which had to be reverted as GCC would reject
> valid
> code because of the pedwarn).
> 
> Instead of using pewarn the patch below adds a new warning (named
> like
> clang's) to warn about these redundant semicolons.
> Btw, clang's warning message "extra ';' after member function
> definition"
> is slightly incorrect because it is also emitted for friend functions
> which are not member-functions. That's why I suggest a different
> wording:
> 
>   Wextra-semi.C:3:9: warning: extra ';' after in-class function
> definition [-Wextra-semi]
>      A() {};
>            ^
>   Wextra-semi.C:4:16: warning: extra ';' after in-class function
> definition [-Wextra-semi]
>      void foo() {};
>                   ^
>   Wextra-semi.C:5:23: warning: extra ';' after in-class function
> definition [-Wextra-semi]
>      friend void bar() {};
>                          ^
> 
> Bootstrapped and regtested on x86_64-pc-linux-gnu.
> 
> OK for stage 1, once GCC 7 has branched?
> Regards,
> Volker
> 
> 
> 2017-04-07  Volker Reichelt  <v.reichelt@netcologne.de>
> 
>         * c.opt (Wextra-semi): New C++ warning flag.
> 
> Index: gcc/c-family/c.opt
> ===================================================================
> --- gcc/c-family/c.opt  (revision 246752)
> +++ gcc/c-family/c.opt  (working copy)
> @@ -504,6 +504,10 @@
>  C ObjC C++ ObjC++ Warning
>  ; in common.opt
>  
> +Wextra-semi
> +C++ Var(warn_extra_semi) Warning
> +Warn about semicolon after in-class function definition.
> +
>  Wfloat-conversion
>  C ObjC C++ ObjC++ Var(warn_float_conversion) Warning LangEnabledBy(C
> ObjC C++ ObjC++,Wconversion)
>  Warn for implicit type conversions that cause loss of floating point
> precision.
> 
> 2017-04-07  Volker Reichelt  <v.reichelt@netcologne.de>
> 
>         * parser.c (cp_parser_member_declaration): Add warning for
>         extra semicolon after in-class function definition.
> 
> Index: gcc/cp/parser.c
> ===================================================================
> --- gcc/cp/parser.c     (revision 246752)
> +++ gcc/cp/parser.c     (working copy)
> @@ -23386,7 +23386,11 @@
>                   token = cp_lexer_peek_token (parser->lexer);
>                   /* If the next token is a semicolon, consume it. 
> */
>                   if (token->type == CPP_SEMICOLON)
> -                   cp_lexer_consume_token (parser->lexer);
> +                   {
> +                     cp_lexer_consume_token (parser->lexer);
> +                     warning (OPT_Wextra_semi, "extra %<;%> "
> +                              "after in-class function definition");
> +                   }

Thanks for posting this.

I'm not a C++ maintainer, but I like the idea (though the patch is
missing at least a doc/invoke.texi change).

A small improvement to this would be to emit a deletion fix-it hint
about the redundant token (so that IDEs have a change of fixing it
easily).

This could be done something like this:

    location_t semicolon_loc
       = cp_lexer_consume_token (parser->lexer)->location;
    gcc_rich_location richloc (semicolon_loc);
    richloc.add_fixit_remove ();
    warning_at_richloc (&richloc, OPT_Wextra_semi,
                        "extra %<;%> after in-class function
definition");


>                   goto out;
>                 }
>               else
> 
> 2017-04-07  Volker Reichelt  <v.reichelt@netcologne.de>
> 
>         * g++.dg/warn/Wextra-semi.C: New test.
> 
> Index: gcc/testsuite/g++.dg/warn/Wextra-semi.C
> ===================================================================
> --- gcc/testsuite/g++.dg/warn/Wextra-semi.C     2017-04-07
> +++ gcc/testsuite/g++.dg/warn/Wextra-semi.C     2017-04-07
> @@ -0,0 +1,8 @@
> +// { dg-options "-Wextra-semi" }
> +
> +struct A
> +{
> +  A() {};              // { dg-warning "after in-class function
> definition" }
> +  void foo() {};       // { dg-warning "after in-class function
> definition" }
> +  friend void bar() {};        // { dg-warning "after in-class
> function definition" }
> +};
> 

If you implement the fix-it idea, then you can add 
-fdiagnostics-show-caret to the dg-options, and use something like:

/* { dg-begin-multiline-output "" }
  A() {};
        ^
        -
   { dg-end-multiline-output "" } */

to express the expected output (copied and pasted from GCC's output). 
 You should omit the commented parts of the lines if you do (otherwise
DejaGnu gets very confused due to them containing dg- directives).

Hope this is constructive
Dave