From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9670 invoked by alias); 9 Apr 2017 20:53:10 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 9651 invoked by uid 89); 9 Apr 2017 20:53:09 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-10.8 required=5.0 tests=BAYES_00,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_ASCII_DIVIDERS,RCVD_IN_DNSWL_LOW,RP_MATCHES_RCVD,SPAM_BODY,SPF_PASS autolearn=ham version=3.3.2 spammy=Hope X-HELO: cc-smtpout3.netcologne.de Received: from cc-smtpout3.netcologne.de (HELO cc-smtpout3.netcologne.de) (89.1.8.213) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sun, 09 Apr 2017 20:53:06 +0000 Received: from cc-smtpin2.netcologne.de (cc-smtpin2.netcologne.de [89.1.8.202]) by cc-smtpout3.netcologne.de (Postfix) with ESMTP id D07B812520; Sun, 9 Apr 2017 22:53:04 +0200 (CEST) Received: from localhost (localhost [127.0.0.1]) by cc-smtpin2.netcologne.de (Postfix) with ESMTP id C1C8411D86; Sun, 9 Apr 2017 22:53:04 +0200 (CEST) Received: from [89.0.0.168] (helo=cc-smtpin2.netcologne.de) by localhost with ESMTP (eXpurgate 4.1.9) (envelope-from ) id 58ea9f30-022c-7f0000012729-7f0000018b16-1 for ; Sun, 09 Apr 2017 22:53:04 +0200 Received: from linux-w03z.fritz.box (xdsl-89-0-0-168.netcologne.de [89.0.0.168]) (using TLSv1 with cipher ECDHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by cc-smtpin2.netcologne.de (Postfix) with ESMTPSA; Sun, 9 Apr 2017 22:53:01 +0200 (CEST) Date: Sun, 09 Apr 2017 20:53:00 -0000 From: Volker Reichelt Subject: Re: [C++ PATCH] New warning for extra semicolons after in-class function definitions To: David Malcolm , gcc-patches@gcc.gnu.org In-Reply-To: <1491600152.9106.9.camel@redhat.com> Message-ID: References: <1491600152.9106.9.camel@redhat.com> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; CHARSET=us-ascii Content-Disposition: INLINE X-SW-Source: 2017-04/txt/msg00426.txt.bz2 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 >> >> * 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 >> >> * 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 >> >> * 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 * 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 * 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 * 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 * 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 "" } */ +}; ===================================================================