From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 13018 invoked by alias); 16 Sep 2016 18:19:26 -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 13008 invoked by uid 89); 16 Sep 2016 18:19:25 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.6 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.2 spammy=talked, overlapping, referenced, hasn't X-HELO: mail-qk0-f175.google.com Received: from mail-qk0-f175.google.com (HELO mail-qk0-f175.google.com) (209.85.220.175) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 16 Sep 2016 18:19:23 +0000 Received: by mail-qk0-f175.google.com with SMTP id h8so90738687qka.1 for ; Fri, 16 Sep 2016 11:19:22 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=J7o9ZZMYkFg7GMu+it3hceiRP2ZMCusD3iadKT3JowI=; b=ZRgJ6JTUFHq14TCTewkZQtRj9sXpfIPgGQRagQdT4jFuge/Y3BLByAqy/cllbga8tI Y8mRMorgp0H9xqwQ2lXUPaGiX1iajrQwLWId4oLd1a93DWCyvaEAPPRhN4glzR81Jzl/ xAK0zW1WmtCFwpQFtzrM2Qr6VlFo6fRsDA11NoWTZpB+57fNoO7aTVONe80EOaoizCFq o/00HD7pdVeWhvwmuiri1pF3gMtKw6WGaLSf++/+ySu4o/EAz99EQtF1yDcqIIFSOCpq /9jOZROxWPBlVPj4s8ydAmyiDTxcsp89FU05qmzPZ+Zedwbgk1obrZfjt22GaKL70zT4 kelw== X-Gm-Message-State: AE9vXwPt8Z6FbfIZk+Lq2S4Qv52/k+7ASct4gkfI3JvpPnMW2Y+e4MywVU/kkJ3maQntxfP3 X-Received: by 10.55.71.17 with SMTP id u17mr18155355qka.144.1474049961483; Fri, 16 Sep 2016 11:19:21 -0700 (PDT) Received: from [192.168.1.149] (209-6-90-240.c3-0.smr-ubr1.sbo-smr.ma.cable.rcn.com. [209.6.90.240]) by smtp.gmail.com with ESMTPSA id 5sm5356903qts.45.2016.09.16.11.19.20 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 16 Sep 2016 11:19:20 -0700 (PDT) Subject: Re: [PATCH] accept flexible arrays in struct in unions (c++/71912 - [6/7 regression]) To: Martin Sebor References: <57927079.5080400@gmail.com> <5793A6F4.2090900@gmail.com> <69dc2b8d-1841-92b7-20ed-6e0a899435f2@redhat.com> <579BE53E.7070002@gmail.com> <6a6e323e-dbbb-f903-2492-e8ed26e01d65@redhat.com> <579E5F1E.8060603@gmail.com> <57A109E6.8010809@gmail.com> <57A15341.9070702@gmail.com> <73604cac-ed57-8811-aaf7-232cab86ac2d@gmail.com> Cc: Gcc Patch List From: Jason Merrill Message-ID: Date: Fri, 16 Sep 2016 18:40:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 In-Reply-To: <73604cac-ed57-8811-aaf7-232cab86ac2d@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2016-09/txt/msg01074.txt.bz2 On 09/14/2016 01:03 PM, Martin Sebor wrote: > Attached is an updated patch that does away with the "overlapping" > warning and instead issues the same warnings as the C front end > (though with more context). > > In struct flexmems_t I've retained the NEXT array even though only > the first element is used to diagnose problems. The second element > is used by the find_flexarrays function to differentiate structs > with flexible array members in unions (which are valid) from those > in structs (which are not). > > FWIW, I think this C restriction on structs is unnecessary and will > propose to remove it so that's valid to define a struct that contains > another struct (possibly recursively) with a flexible array member. > I.e., I think this should be made valid in C (and accepted without > the pedantic warning that GCC, and with this patch also G++, issues): Agreed. > + /* Is FLD a typedef for an anonymous struct? */ > + bool anon_type_p > + = (TREE_CODE (fld) == TYPE_DECL > + && DECL_IMPLICIT_TYPEDEF_P (fld) > + && anon_aggrname_p (DECL_NAME (fld))); We talked earlier about handling typedefs in cp_parser_{simple,single}_declaration, so that we catch typedef struct { int a[]; } B; or at least adding a FIXME comment here explaining that looking at typedefs is a temporary hack. > + /* Type of the member. */ > + tree fldtype = TREE_CODE (fld) == FIELD_DECL ? TREE_TYPE (fld) : fld; Why set "fldtype" to be a TYPE_DECL rather than its type? > + /* Determine the type of the array element or object referenced > + by the member so that it can be checked for flexible array > + members if it hasn't been yet. */ > + tree eltype = TREE_CODE (fld) == FIELD_DECL ? fldtype : TREE_TYPE (fld); Given the above, this seems equivalent to tree eltype = TREE_TYPE (fld); > + if (RECORD_OR_UNION_TYPE_P (eltype)) > + { ... > + if (fmem->array && !fmem->after[bool (pun)]) > + { > + /* Once the member after the flexible array has been found > + we're done. */ > + fmem->after[bool (pun)] = fld; > + break; > + } ... > if (field_nonempty_p (fld)) > { ... > /* Remember the first non-static data member after the flexible > array member, if one has been found, or the zero-length array > if it has been found. */ > if (fmem->array && !fmem->after[bool (pun)]) > fmem->after[bool (pun)] = fld; > } Can we do this in one place rather than two? > + if (eltype == fldtype || TYPE_UNNAMED_P (eltype)) This is excluding arrays, so we don't diagnose, say, > struct A > { > int i; > int ar[]; > }; > > struct B { > A a[2]; > }; Should we complain elsewhere about an array of a class with a flexible array member? > + /* Does the field represent an anonymous struct? */ > + bool anon_p = !DECL_NAME (fld) && ANON_AGGR_TYPE_P (eltype); You don't need to check DECL_NAME; ANON_AGGR_TYPE_P by itself indicates that we're dealing with an anonymous struct/union. > Similarly, PSTR is set to the outermost struct of which T is a member > if one such struct exists, otherwise to NULL. */ ... > find_flexarrays (eltype, fmem, false, pun, > !pstr && TREE_CODE (t) == RECORD_TYPE ? fld : pstr); ... > +diagnose_invalid_flexarray (const flexmems_t *fmem) > +{ > + if (fmem->array && fmem->enclosing > + && pedwarn (location_of (fmem->enclosing), OPT_Wpedantic, > + TYPE_DOMAIN (TREE_TYPE (fmem->array)) > + ? G_("invalid use of %q#T with a zero-size array " > + "in %q#D") > + : G_("invalid use of %q#T with a flexible array member " > + "in %q#T"), > + DECL_CONTEXT (fmem->array), > + DECL_CONTEXT (fmem->enclosing))) PSTR is documented to be the outermost struct, but it (and thus fmem->enclosing) end up being a data member of that outermost struct, which is why you need to take its DECL_CONTEXT. What's the advantage of doing that over passing t itself to the recursive call? > + /* The context of the flexible array member. Either the struct > + in which it's declared or, for anonymous structs and unions, > + the struct/union of which the array is effectively a member. */ > + tree fmemctx = anon_context (fmem->array); > + bool anon_p = fmemctx != DECL_CONTEXT (fmem->array); > + if (!anon_p) > + fmemctx = t; Why do you need to do something different here based on anon_p? I don't see why that would affect whether we want to look through intermediate non-anonymous classes. > + check_flexarrays (basetype, fmem, true); Please decorate boolean literal arguments like this with the name of the parameter, e.g. /*base_p*/true. > + bool maybe_anon_p = anon_aggrname_p (TYPE_IDENTIFIER (t)); Now we can use TYPE_UNNAMED_P. Jason