From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 112923 invoked by alias); 8 May 2015 21:45:13 -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 112906 invoked by uid 89); 8 May 2015 21:45:13 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.8 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Fri, 08 May 2015 21:45:12 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t48LjA6G021637 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Fri, 8 May 2015 17:45:10 -0400 Received: from localhost.localdomain (ovpn-113-143.phx2.redhat.com [10.3.113.143]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t48Lj9Ze005897; Fri, 8 May 2015 17:45:10 -0400 Message-ID: <554D2E65.4010802@redhat.com> Date: Fri, 08 May 2015 21:45:00 -0000 From: Jeff Law User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: David Malcolm CC: gcc-patches@gcc.gnu.org Subject: Re: [PATCH 2/4 v2] libcpp: Replace macro usage with C++ constructs) References: <5547C538.5000606@redhat.com> <1430850074-40522-1-git-send-email-dmalcolm@redhat.com> In-Reply-To: <1430850074-40522-1-git-send-email-dmalcolm@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2015-05/txt/msg00761.txt.bz2 On 05/05/2015 12:21 PM, David Malcolm wrote: > On Mon, 2015-05-04 at 13:15 -0600, Jeff Law wrote: > On 05/01/2015 06:56 PM, David Malcolm wrote: >>> >>> [I didn't mark the inline functions as "static"; should they be?] Just a follow-up on this. I got burned by the ODR issues around implicit extern inlines earlier this week. Basically I had two distinct implementations for an inline function with the same name. They obviously appeared in different "header files". When optimizng, this was fine as they'd actually get inlined and all was good. Things blew up when the optimizer was off. We got two functions with the same name, but different implementations. The linker blindly chose one for me, and in one context it was the wrong one. This led to bootstrap comparison failures. So, just be careful :-) >> > I moved linemap_assert and linemap_assert_fails higher up within the > file so that they can appear before the bodies of the inline functions. > I didn't change their implementation; it's a simple cut-and-paste, so > these hunks are just an artifact of git's diff-ing algorithm getting > confused by that. > > Should I have called that out in the ChangeLog entries? It might have helped. Or you could have pulled it out like you did in the follow-up. No worries. So consider the issues raised in that code as a non-issues for your patch. They'd be nice things to clean up someday though. >> -#define LINEMAPS_MAPS(SET, MAP_KIND) \ >>> - (LINEMAPS_MAP_INFO (SET, MAP_KIND))->maps >>> +inline line_map * >>> +LINEMAPS_MAPS (const line_maps *set, bool map_kind) >>> +{ >>> + return LINEMAPS_MAP_INFO (set, map_kind)->maps; >>> +} >>> + >>> +inline line_map * & >>> +LINEMAPS_MAPS (line_maps *set, bool map_kind) >>> +{ >>> + return LINEMAPS_MAP_INFO (set, map_kind)->maps; >>> +} >> Do we really need two implementation of those various functions, one for >> const line_maps * the other for line_maps *? >> > > The issue here is when they are accessed as lvalues, rather than just as > rvalues. > > I've had a go at eliminating some of the lvalue ones as a followup > patch: given that after patch 4 these become just field accesses, it's > trivial to turn the usage sites into plain accesses of fields. > > Is that desirable, or are the uses of the macros regarded as useful > (e.g. for grepping)? (my preference is for the former: to turn them > into plain field accesses at those sites) I think macros would only be useful for those already familiar with the code. If I'm looking for how a particular field gets accessed, I'm generally going to be grepping for the field. Of course, if the field have names that are common ("next" anyone?), then, well, grepping for field names is pointless. > > Ironically, one of the ones that isn't so easy to eliminate is the > one you called out above. Eliminating those ones made the code messier, > so I kept those ones. lol. I just picked one after seeing the pattern repeat a bit. > > I'm sending updated patches; the old patch 2 split into 2 parts, and > a followup ("patch 5 of 4", as it were): > > [PATCH 2/4 v2: part 1] Move linemap_assert higher up within the file > [PATCH 2/4 v2: part 2] libcpp: Replace macro usage with C++ constructs > [PATCH 5/4] libcpp: Eliminate most of the non-const/reference-returning inline fns > > Do I need to do any performance testing of this kit? Given that the > resulting inline functions become trivial field lookups in patch 4, > I'm assuming that our inliner is good enough to optimize it as well > as the status quo implementation. Yea, I wouldn't worry about performance of an inline field access vs a macro. We should be good to go. > > Also, for comments before functions, do we prefer a blank line between > the comment and decl, or for them to abutt? > > /* Foo. */ > > foo (); > > vs > > /* Bar. */ > bar (); > > How do the following look? I've done both in my days. I try to follow whatever is in the nearby code. Let me have a look at the followup... jeff