On 7 Apr, David Malcolm wrote:
> 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 <[email protected]>
>>
>> * 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 <[email protected]>
>>
>> * 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).
You're right. I had the nagging feeling that I forgot something.
I added a respective section to doc/invoke.texi after -Wempty-body
and -Wenum-compare.
> 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");
>
That's a nice suggestion. I modified the patch accordingly.
>> goto out;
>> }
>> else
>>
>> 2017-04-07 Volker Reichelt <[email protected]>
>>
>> * 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).
Even without the fix-it, the multiline check would make sense for the
position of the caret, so I followed your suggestion.
> Hope this is constructive
> Dave
Thanks a lot for your very helpful comments, Dave!
So here's an updated version of the patch, again bootstrapped
and regtested on x86_64-pc-linux-gnu.
Right now, the patch enables the warning only for C++.
Would it make sense to enable it also for ObjC++?
OK for stage 1 (with or without ObjC++ support), once GCC 7 has branched?
Regards,
Volker
2017-04-09 Volker Reichelt <[email protected]>
* 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-09 Volker Reichelt <[email protected]>
* doc/invoke.texi (-Wextra-semi): Document new warning option.
Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi (revision 246752)
+++ gcc/doc/invoke.texi (working copy)
@@ -274,7 +274,8 @@
-Wno-discarded-qualifiers -Wno-discarded-array-qualifiers @gol
-Wno-div-by-zero -Wdouble-promotion -Wduplicated-cond @gol
-Wempty-body -Wenum-compare -Wno-endif-labels -Wexpansion-to-defined @gol
--Werror -Werror=* -Wfatal-errors -Wfloat-equal -Wformat -Wformat=2 @gol
+-Werror -Werror=* -Wextra-semi -Wfatal-errors @gol
+-Wfloat-equal -Wformat -Wformat=2 @gol
-Wno-format-contains-nul -Wno-format-extra-args @gol
-Wformat-nonliteral -Wformat-overflow=@var{n} @gol
-Wformat-security -Wformat-signedness -Wformat-truncation=@var{n} @gol
@@ -5960,6 +5961,11 @@
diagnosed and the warning is enabled by default. In C this warning is
enabled by @option{-Wall}.
+@item -Wextra-semi @r{(C++ only)}
+@opindex Wextra-semi
+@opindex Wno-extra-semi
+Warn about redundant semicolon after in-class function definition.
+
@item -Wjump-misses-init @r{(C, Objective-C only)}
@opindex Wjump-misses-init
@opindex Wno-jump-misses-init
2017-04-09 Volker Reichelt <[email protected]>
* parser.c (cp_parser_member_declaration): Add warning with fixit
information 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,15 @@
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);
+ {
+ location_t semicolon_loc
+ = cp_lexer_consume_token (parser->lexer)->location;
+ gcc_rich_location richloc (semicolon_loc);
+ richloc.add_fixit_remove ();
+ warning_at_rich_loc (&richloc, OPT_Wextra_semi,
+ "extra %<;%> after in-class "
+ "function definition");
+ }
goto out;
}
else
2017-04-09 Volker Reichelt <[email protected]>
* 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-09
+++ gcc/testsuite/g++.dg/warn/Wextra-semi.C 2017-04-09
@@ -0,0 +1,25 @@
+// { dg-options "-Wextra-semi -fdiagnostics-show-caret" }
+
+struct A
+{
+ A() {}; /* { dg-warning "after in-class function definition" }
+ { dg-begin-multiline-output "" }
+ A() {};
+ ^
+ -
+ { dg-end-multiline-output "" } */
+
+ void foo() {}; /* { dg-warning "after in-class function definition" }
+ { dg-begin-multiline-output "" }
+ void foo() {};
+ ^
+ -
+ { dg-end-multiline-output "" } */
+
+ friend void bar() {}; /* { dg-warning "after in-class function definition" }
+ { dg-begin-multiline-output "" }
+ friend void bar() {};
+ ^
+ -
+ { dg-end-multiline-output "" } */
+};
===================================================================
