From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27983 invoked by alias); 21 Jul 2017 17:17:27 -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 27961 invoked by uid 89); 21 Jul 2017 17:17:26 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-7.6 required=5.0 tests=BAYES_00,GIT_PATCH_2,RCVD_IN_DNSWL_LOW,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=favorite X-HELO: cc-smtpout2.netcologne.de Received: from cc-smtpout2.netcologne.de (HELO cc-smtpout2.netcologne.de) (89.1.8.212) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 21 Jul 2017 17:17:24 +0000 Received: from cc-smtpin1.netcologne.de (cc-smtpin1.netcologne.de [89.1.8.201]) by cc-smtpout2.netcologne.de (Postfix) with ESMTP id 6524A1258A; Fri, 21 Jul 2017 19:17:20 +0200 (CEST) Received: from localhost (localhost [127.0.0.1]) by cc-smtpin1.netcologne.de (Postfix) with ESMTP id 57BAB11DC4; Fri, 21 Jul 2017 19:17:20 +0200 (CEST) Received: from [89.0.10.135] (helo=cc-smtpin1.netcologne.de) by localhost with ESMTP (eXpurgate 4.1.9) (envelope-from ) id 59723720-021e-7f0000012729-7f000001a6db-1 for ; Fri, 21 Jul 2017 19:17:20 +0200 Received: from linux-mqtz.fritz.box (xdsl-89-0-10-135.netcologne.de [89.0.10.135]) (using TLSv1 with cipher ECDHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by cc-smtpin1.netcologne.de (Postfix) with ESMTPSA; Fri, 21 Jul 2017 19:17:18 +0200 (CEST) Date: Fri, 21 Jul 2017 17:17:00 -0000 From: Volker Reichelt Subject: Re: [PATCH] New C++ warning option '-Wduplicated-access-specifiers' To: David Malcolm cc: gcc-patches List In-Reply-To: <1500655652.7686.37.camel@redhat.com> Message-ID: References: <1500652589.7686.31.camel@redhat.com> <1500655652.7686.37.camel@redhat.com> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; CHARSET=us-ascii Content-Disposition: INLINE X-SW-Source: 2017-07/txt/msg01325.txt.bz2 On 21 Jul, David Malcolm wrote: > On Fri, 2017-07-21 at 11:56 -0400, David Malcolm wrote: >> On Thu, 2017-07-20 at 18:35 +0200, Volker Reichelt wrote: >> > Hi, >> > >> > the following patch introduces a new C++ warning option >> > -Wduplicated-access-specifiers that warns about redundant >> > access-specifiers in classes, e.g. >> > >> > class B >> > { >> > public: >> > B(); >> > >> > private: >> > void foo(); >> > private: >> > int i; >> > }; >> > >> > test.cc:8:5: warning: duplicate 'private' access-specifier [ >> > -Wduplicated-access-specifiers] >> > private: >> > ^~~~~~~ >> > ------- >> > test.cc:6:5: note: access-specifier was previously given here >> > private: >> > ^~~~~~~ >> >> Thanks for working on this. >> >> I'm not able to formally review, but you did CC me; various comments >> below throughout. >> >> > The test is implemented by tracking the location of the last >> > access-specifier together with the access-specifier itself. >> > The location serves two purposes: the obvious one is to be able to >> > print the location in the note that comes with the warning. >> > The second purpose is to be able to distinguish an access-specifier >> > given by the user from the default of the class type (i.e. public >> > for >> > 'struct' and private for 'class') where the location is set to >> > UNKNOWN_LOCATION. The warning is only issued if the user gives the >> > access-specifier twice, i.e. if the previous location is not >> > UNKNOWN_LOCATION. >> >> Presumably given >> >> struct foo >> { >> public: >> /* ... * >> }; >> >> we could issue something like: >> >> warning: access-specifier 'public' is redundant within 'struct' >> >> for the first; similarly for: >> >> class bar >> { >> private: >> }; >> >> we could issue: >> >> warning: access-specifier 'private' is redundant within 'class' >> >> >> > One could actually make this a two-level warning so that on the >> > higher level also the default class-type settings are taken into >> > account. Would that be helpful? I could prepare a second patch for >> > that. >> >> I'm not sure how best to structure it. > > Maybe combine the two ideas, and call it > -Wredundant-access-specifiers > ? > > Or just "-Waccess-specifiers" ? > > [...snip...] > > Dave Thanks for having a look at this! My favorite version would be to use '-Waccess-specifiers=1' for the original warning and '-Waccess-specifiers=2' for the stricter version that also checks against the class-type default. We could then let '-Waccess-specifiers' default to any of those two. I'm afraid that level 2 will fire way too often for legacy code (and there might be coding conventions that require an access specifier at the beginning of a class/struct). So I'd vote for level 1 as default. The last argument is also the reason why I'd like to see a two-lveel warning instead of just different error messages (although these are very helpful for seeing what's really goiing on with your code). What do you think? Regards, Volker