From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-74.mimecast.com (us-smtp-delivery-74.mimecast.com [216.205.24.74]) by sourceware.org (Postfix) with ESMTP id 3B42B385E013 for ; Thu, 26 Mar 2020 22:16:37 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 3B42B385E013 Received: from mail-qv1-f71.google.com (mail-qv1-f71.google.com [209.85.219.71]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-139-nDg43QePPc6hJXZ6EMQ11g-1; Thu, 26 Mar 2020 18:16:28 -0400 X-MC-Unique: nDg43QePPc6hJXZ6EMQ11g-1 Received: by mail-qv1-f71.google.com with SMTP id u19so6082244qvv.9 for ; Thu, 26 Mar 2020 15:16:28 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=KWJj7ymq1eLbotRTfokkhoNQT3/e0JENPENdyE9FUl4=; b=JqhkiL+9TlhCzSL+iG5SmZk2+rfJk5jYbvX0Tk/g9caAIRXc/VQ7nK34CV2SF1BCtg QDFDpvvqnHYJRGNGF24/lK15f5gTatQH+DFCju7Moan42ZKh7VPBChuehHCPL1jxIVsm oM6ZnJ/Fxsqi8p0iZn6OGXamZTEwvOqgI69UQ+l+VqcIvISuNKWZ4s9cnDnrLGjrJkwK 0LLPTL8cC4z/mSmVDcRUiLbEoUH+IQtdIHF+KLR9Vhma2ZnYw59KnfavXvp9dV1LmtJ/ vlkJ1d/zTp/tAGjVwIcM3f9t6UdjoxN6NPqFLEBQrcQ5/3a6uD30EyRxarTH3GEYtXEo Zo6g== X-Gm-Message-State: ANhLgQ1gE/HuAPAWc+f9InVueQhH7Lkc1uEXUmN+egtVI1AHAdctujrU g021yhLqsA+F2y6zBdawelCs3OsFam7M1Pt2UQNtLPMe2rhAqJJa/UW4cNBxO4TyTQr+cDA6QV8 2ZKHKtgjzW+NbPVYXew== X-Received: by 2002:ac8:668d:: with SMTP id d13mr11186383qtp.362.1585260987723; Thu, 26 Mar 2020 15:16:27 -0700 (PDT) X-Google-Smtp-Source: ADFU+vtW/IqB4/qhzQJaJ6AA+uA9WaP2O+h5IDT7uo0vfB2gLeYXM2toQq75cK5nffgCSoqBa1HCvA== X-Received: by 2002:ac8:668d:: with SMTP id d13mr11186347qtp.362.1585260987307; Thu, 26 Mar 2020 15:16:27 -0700 (PDT) Received: from [192.168.1.148] (209-6-216-142.s141.c3-0.smr-cbr1.sbo-smr.ma.cable.rcncustomer.com. [209.6.216.142]) by smtp.gmail.com with ESMTPSA id q7sm2368073qti.58.2020.03.26.15.16.26 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 26 Mar 2020 15:16:26 -0700 (PDT) Subject: Re: [PATCH] avoid -Wredundant-tags on a first declaration in use (PR 93824) To: Martin Sebor , gcc-patches References: <2698a399-4176-2b5f-a134-52a0d82c2121@redhat.com> <13b8faa9-74b1-6131-5dda-d4f34dfa8af0@gmail.com> <77dc94af-40f1-46fd-f3f4-ceb7e3afc4b6@gmail.com> <00f21176-ea92-937c-5c8a-c04d5d813257@redhat.com> <90a6aebc-8fef-0b96-cd2b-3234bd82b9a0@gmail.com> <2c553c0d-c0ac-88ba-2a85-12c523a9a164@redhat.com> <2744364d-e705-06f8-064a-5425ade11669@gmail.com> <1239c5f0-7ea7-e760-0b1c-1fe725a5e129@redhat.com> <48d42e59-64c4-6674-40a4-c786f9e504f4@gmail.com> <6775e196-9f97-6651-bc6a-ee6d4b7d16bc@gmail.com> <3636dd58-f935-bd92-4b20-003950980b46@redhat.com> From: Jason Merrill Message-ID: <2ee80974-c9dd-bce6-59ce-e4def7dd3e38@redhat.com> Date: Thu, 26 Mar 2020 18:16:25 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.3.0 MIME-Version: 1.0 In-Reply-To: Content-Language: en-US X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-19.6 required=5.0 tests=BAYES_00, BODY_8BITS, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GARBLED_BODY, GIT_PATCH_1, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 26 Mar 2020 22:16:38 -0000 On 3/26/20 2:58 PM, Martin Sebor wrote: > On 3/25/20 11:36 PM, Jason Merrill wrote: >> On 3/23/20 12:50 PM, Martin Sebor wrote: >>> On 3/23/20 8:49 AM, Jason Merrill wrote: >>>> On 3/21/20 5:59 PM, Martin Sebor wrote: >>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* Diagnose class/struct/union mismat= ches.=C2=A0 IS_DECLARATION=20 >>>>> is false >>>>> +=C2=A0=C2=A0=C2=A0=C2=A0 for alias definition.=C2=A0 */ >>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 bool decl_class =3D (is_declaration >>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0 && cp_parser_declares_only_class_p (parser)); >>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 cp_parser_check_class_key = (parser, key_loc, tag_type, type,=20 >>>>> false, >>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 cp_parser_declares_only_class_p (pa= rser)); >>>> >>>> Don't you need to use the new variable? >>>> >>>> Don't your testcases exercise this? >>> >>> Of course they do.=C2=A0 This was a leftover from an experiment after I= put >>> the initial updated patch together.=C2=A0 On final review I decided to = adjust >>> some comments and forgot to restore the original use of the variable. >>> >>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* When TYPE is the use of an implici= t specialization of a=20 >>>>> previously >>>>> +=C2=A0=C2=A0=C2=A0=C2=A0 declared template set TYPE_DECL to the type= of the primary=20 >>>>> template >>>>> +=C2=A0=C2=A0=C2=A0=C2=A0 for the specialization and look it up in CL= ASS2LOC below. =20 >>>>> For uses >>>>> +=C2=A0=C2=A0=C2=A0=C2=A0 of explicit or partial specializations TYPE= _DECL already=20 >>>>> points to >>>>> +=C2=A0=C2=A0=C2=A0=C2=A0 the declaration of the specialization. >>>>> +=C2=A0=C2=A0=C2=A0=C2=A0 IS_USE is clear so that the type of an impl= icit instantiation=20 >>>>> rather >>>>> +=C2=A0=C2=A0=C2=A0=C2=A0 than that of a partial specialization is de= termined.=C2=A0 */ >>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 type_decl =3D TREE_TYPE (type_decl); >>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (TREE_CODE (type_decl) !=3D TEMPLA= TE_DECL) >>>>> +=C2=A0=C2=A0=C2=A0 type_decl =3D TYPE_MAIN_DECL (type_decl); >>>> >>>> The comment is no longer relevant to the code.=C2=A0 The remaining cod= e=20 >>>> also seems like it would have no effect; we already know type_decl=20 >>>> is TYPE_MAIN_DECL (type). >>> >>> I removed the block of code. >>> >>> Martin >>> >>> PS I would have preferred to resolve just the reported problem in this >>> patch and deal with the template specializations more fully (and with >>> aliases) in a followup.=C2=A0 As it is, it has grown bigger and more co= mplex >>> than I'm comfortable with, especially with the template specializations= , >>> harder for me to follow, and obviously a lot more time-consuming not >>> just to put together but also to review.=C2=A0 Although this revision h= andles >>> many more template specialization cases correctly, there still are othe= r >>> (arguably corner) cases that it doesn't.=C2=A0 I suspect getting those = right >>> might even require a design change, which I see as out of scope at this >>> time (not to mention my ability). >> >> Sure, at this point in the cycle there's always a tradeoff between=20 >> better functionality and risk from ballooning changes.=C2=A0 It looks li= ke=20 >> the improved template handling could still be split out into a=20 >> separate patch, if you'd prefer. >=20 > I would prefer to get this patch committed as is now.=C2=A0 I appreciate > there are improvements that can be made to the code (there always > are) but, unlike the bugs it fixes, they are invisible to users and > so don't seem essential at this point. >=20 >>> +=C2=A0 /* Number of usesn of the class.=C2=A0 */ >> Typo. >> >>> +=C2=A0=C2=A0=C2=A0=C2=A0 definintion if one exists or the first declar= ation otherwise.=C2=A0 */ >> typo. >> >>> +=C2=A0 if (CLASSTYPE_USE_TEMPLATE (type) =3D=3D 1 && !is_decl (0)) >> ... >>> +=C2=A0=C2=A0=C2=A0=C2=A0 the first reference to the instantiation.=C2= =A0 The primary must >>> +=C2=A0=C2=A0=C2=A0=C2=A0 be (and inevitably is) at index zero.=C2=A0 *= / >> >> I think CLASSTYPE_IMPLICIT_INSTANTIATION is more readable than testing= =20 >> the value 1. >=20 > Okay. >=20 >> >> What is the !is_decl (0) intended to do?=C2=A0 Changing it to an assert= =20 >> inside the 'if' doesn't seem to affect any of the testcases. >=20 > Looks like the test is an unnecessary remnant and can be removed. > In fact, both is_decl() and decl_p member don't appear necessary > anymore so I've removed them too. >=20 >>> +=C2=A0=C2=A0=C2=A0=C2=A0 For implicit instantiations of a primary temp= late it's >>> +=C2=A0=C2=A0=C2=A0=C2=A0 the class-key used to declare the primary wit= h.=C2=A0 The primary >>> +=C2=A0=C2=A0=C2=A0=C2=A0 must be at index zero.=C2=A0 */ >>> +=C2=A0 const tag_types xpect_key >>> +=C2=A0=C2=A0=C2=A0 =3D cdlguide->class_key (cdlguide =3D=3D this ? idx= guide : 0); >> >> A template can also be declared before it's defined; >=20 > Obviously, just like a class.=C2=A0 Is there something you expect me to > change in response to this point? You're hardcoding index zero for the template case, which seems to=20 assume that the definition of a template is always at index zero. >> I think you want to move the def_p/idxdef/idxguide logic into another=20 >> member function that you invoke on cdlguide to perhaps get the=20 >> class_key_loc_t that you want to use as the pattern. >=20 > I'm not quite sure what you have in mind here.=C2=A0 I agree the cdlcode > code looks a little cumbersome and perhaps could be restructured but > it's not obvious to me how.=C2=A0 Nothing I tried looked like a clear win > so unless you consider changing how this is done a prerequisite for > accepting the whole patch I'd rather not spend any more time at this > stage iterating over refinements of it.=C2=A0 Please let me know soon. I mean that > + const unsigned ndecls =3D locvec.length (); > + const bool def_p =3D = idxdef < ndecls; > + const unsigned idxguide =3D def_p ? idxdef : 0; are based on whether the instantiation, rather than the template, is=20 defined. I's probably enough to update ndecls to cdlguide->locvec.length() and=20 change the uses of idxdef to cdlguide->idxdef. And then idxguide will=20 be set properly for cdlguide, so the code farther above can become > + const tag_types xpect_key > + =3D cdlguide->class_key (idxguide); If you'd prefer, I can make these changes and commit the patch myself. Jason