From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 110318 invoked by alias); 5 May 2015 18:08: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 110305 invoked by uid 89); 5 May 2015 18:08:10 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 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; Tue, 05 May 2015 18:08:08 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (Postfix) with ESMTPS id 1C6D0B8127 for ; Tue, 5 May 2015 18:08:07 +0000 (UTC) Received: from c64.redhat.com (vpn-225-80.phx2.redhat.com [10.3.225.80]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t45I86la012150; Tue, 5 May 2015 14:08:06 -0400 From: David Malcolm To: Jeff Law Cc: gcc-patches@gcc.gnu.org, David Malcolm Subject: [PATCH 2/4 v2] libcpp: Replace macro usage with C++ constructs) Date: Tue, 05 May 2015 18:08:00 -0000 Message-Id: <1430850074-40522-1-git-send-email-dmalcolm@redhat.com> In-Reply-To: <5547C538.5000606@redhat.com> References: <5547C538.5000606@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2015-05/txt/msg00358.txt.bz2 On Mon, 2015-05-04 at 13:15 -0600, Jeff Law wrote: On 05/01/2015 06:56 PM, David Malcolm wrote: > > libcpp makes extensive use of the C preprocessor. Whilst this has a > > pleasingly self-referential quality, I find the code hard-to-read; > > implementing source location support in my JIT branch was much harder than > > I felt it should have been. > > > > In an attempt at making the code easier to follow, and to build towards > > a followup patch, this patch converts most of these macros to C++ > > equivalents: using "const" for compile-time constants, and inline > > functions where macros aren't used as lvalues. > > > > This effectively documents the expected types of the params, and makes > > them available from the debugger e.g.: > > > > (gdb) p LINEMAP_FILE ($3) > > $1 = 0x13b8b37 "" > > > > and indeed the constants also: > > > > (gdb) p IS_ADHOC_LOC(MAX_SOURCE_LOCATION) > > $2 = false > > (gdb) p IS_ADHOC_LOC(MAX_SOURCE_LOCATION + 1) > > $3 = true > > > > [I didn't mark the inline functions as "static"; should they be?] > > > > [FWIW, I posted a reduced version of this patch about a year ago as: > > https://gcc.gnu.org/ml/gcc-patches/2014-05/msg01092.html > > which covered a smaller subset of the macros]. > > > > libcpp/ChangeLog: > > * include/line-map.h (MAX_SOURCE_LOCATION): Convert from a macro > > to a const source_location. > > (RESERVED_LOCATION_COUNT): Likewise. > > (linemap_check_ordinary): Convert from a macro to a pair of inline > > functions, for const/non-const arguments. > > (MAP_START_LOCATION): Likewise. > > (ORDINARY_MAP_STARTING_LINE_NUMBER): Likewise. > > (ORDINARY_MAP_INCLUDER_FILE_INDEX): Likewise. > > (ORDINARY_MAP_IN_SYSTEM_HEADER_P): Likewise. > > (ORDINARY_MAP_NUMBER_OF_COLUMN_BITS): Convert from a macro to a > > pair of inline functions, for const/non-const arguments, where the > > latter is named... > > (SET_ORDINARY_MAP_NUMBER_OF_COLUMN_BITS): New function. > > (ORDINARY_MAP_FILE_NAME): Convert from a macro to a pair of inline > > functions, for const/non-const arguments. > > (MACRO_MAP_MACRO): Likewise. > > (MACRO_MAP_NUM_MACRO_TOKENS): Likewise. > > (MACRO_MAP_LOCATIONS): Likewise. > > (MACRO_MAP_EXPANSION_POINT_LOCATION): Likewise. > > (LINEMAPS_MAP_INFO): Likewise. > > (LINEMAPS_MAPS): Likewise. > > (LINEMAPS_ALLOCATED): Likewise. > > (LINEMAPS_USED): Likewise. > > (LINEMAPS_CACHE): Likewise. > > (LINEMAPS_ORDINARY_CACHE): Likewise. > > (LINEMAPS_MACRO_CACHE): Likewise. > > (LINEMAPS_MAP_AT): Convert from a macro to an inline function. > > (LINEMAPS_LAST_MAP): Likewise. > > (LINEMAPS_LAST_ALLOCATED_MAP): Likewise. > > (LINEMAPS_ORDINARY_MAPS): Likewise. > > (LINEMAPS_ORDINARY_MAP_AT): Likewise. > > (LINEMAPS_ORDINARY_ALLOCATED): Likewise. > > (LINEMAPS_ORDINARY_USED): Likewise. > > (LINEMAPS_LAST_ORDINARY_MAP): Likewise. > > (LINEMAPS_LAST_ALLOCATED_ORDINARY_MAP): Likewise. > > (LINEMAPS_MACRO_MAPS): Likewise. > > (LINEMAPS_MACRO_MAP_AT): Likewise. > > (LINEMAPS_MACRO_ALLOCATED): Likewise. > > (LINEMAPS_MACRO_USED): Likewise. > > (LINEMAPS_MACRO_LOWEST_LOCATION): Likewise. > > (LINEMAPS_LAST_MACRO_MAP): Likewise. > > (LINEMAPS_LAST_ALLOCATED_MACRO_MAP): Likewise. > > (IS_ADHOC_LOC): Likewise. > > (COMBINE_LOCATION_DATA): Likewise. > > (SOURCE_LINE): Likewise. > > (SOURCE_COLUMN): Likewise. > > (LAST_SOURCE_LINE_LOCATION): Likewise. > > (LAST_SOURCE_LINE): Likewise. > > (LAST_SOURCE_COLUMN): Likewise. > > (LAST_SOURCE_LINE_LOCATION) > > (INCLUDED_FROM): Likewise. > > (MAIN_FILE_P): Likewise. > > (LINEMAP_FILE): Likewise. > > (LINEMAP_LINE): Likewise. > > (LINEMAP_SYSP): Likewise. > > (linemap_location_before_p): Likewise. > > * line-map.c (linemap_check_files_exited): Make local "map" const. > > (linemap_add): Use SET_ORDINARY_MAP_NUMBER_OF_COLUMN_BITS. > > (linemap_line_start): Likewise. > > --- > > -#define MAP_START_LOCATION(MAP) (MAP)->start_location > > +#if defined ENABLE_CHECKING && (GCC_VERSION >= 2007) > > + > > +/* Assertion macro to be used in line-map code. */ > > +#define linemap_assert(EXPR) \ > > + do { \ > > + if (! (EXPR)) \ > > + abort (); \ > > + } while (0) > > + > > +/* Assert that becomes a conditional expression when checking is disabled at > > + compilation time. Use this for conditions that should not happen but if > > + they happen, it is better to handle them gracefully rather than crash > > + randomly later. > > + Usage: > > + > > + if (linemap_assert_fails(EXPR)) handle_error(); */ > > +#define linemap_assert_fails(EXPR) __extension__ \ > > + ({linemap_assert (EXPR); false;}) > > + > > +#else > > +/* Include EXPR, so that unused variable warnings do not occur. */ > > +#define linemap_assert(EXPR) ((void)(0 && (EXPR))) > > +#define linemap_assert_fails(EXPR) (! (EXPR)) > > +#endif > 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? I've split patch 2 of the original kit into two parts, one containing the move of these macros (and the decl of linemap_macro_expansion_map_p) to higher up within the file, then a followup patch containing the "real" changes. (assuming they're OK, should I commit them separately, for ease of seeing this in our SVN history?) > So if we're generally trying to get away from #define programming, then > this part seems like a bit of a step backwards. > > The definition of linemap_assert in the #else clause seems odd. Aren't > you worried that the compiler will short-circuit the test? If EXPR > has side effects, then that's clearly not good. > The (0 && ....) part of this was added in r217473 (aka 36a4e8f0351cdac4befb259ad9ef1738498cdf4e): 2014-11-13 Manuel López-Ibáñez * include/line-map.h: Include EXPR, so that unused variable warnings do not occur. which was due to: https://gcc.gnu.org/ml/gcc-patches/2014-11/msg01240.html > I note you've got an abort () in here, that's generally frowned upon in > libraries. > Agreed, though it's an assertion; it's not clear to me what else we could do. As noted above, I'm not changing this code, simply moving it higher up in the header. > -#define ORDINARY_MAP_FILE_NAME(MAP) \ > > - linemap_check_ordinary (MAP)->d.ordinary.to_file > > +/* Return TRUE if MAP encodes locations coming from a macro > > + replacement-list at macro expansion point. */ > > +bool linemap_macro_expansion_map_p (const struct line_map *); > > + > > +/* Assert that MAP encodes locations of tokens that are not part of > > + the replacement-list of a macro expansion. */ > > +inline struct line_map *linemap_check_ordinary (struct line_map *map) > Return type and inline declarations on a different line than the > function name/arguments. > (nods; will fix) > -#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) 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. > While I suspect ICF ultimately should merge these functions, I'd really > prefer to avoid the duplication. > > Note that many of the macros didn't have comments (they should have, but > it's often missed), correcting that as you turn them into functions > would be greatly appreciated. > > Generally, I like what I see. There's some minor questions/issues to > address, so let's go with another iteration. > > jeff Thanks. 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. 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? Thanks Dave gcc/ada/gcc-interface/trans.c | 2 +- gcc/c-family/c-common.h | 4 +- gcc/c-family/c-lex.c | 6 +- gcc/c-family/c-opts.c | 14 +- gcc/c-family/c-ppoutput.c | 6 +- gcc/common.opt | 4 + gcc/diagnostic.c | 2 +- gcc/fortran/cpp.c | 12 +- gcc/genmatch.c | 4 +- gcc/input.c | 244 ++++++++++++++- gcc/input.h | 2 + gcc/java/jcf-parse.c | 2 +- gcc/toplev.c | 3 + gcc/tree-diagnostic.c | 9 +- libcpp/directives.c | 17 +- libcpp/files.c | 2 +- libcpp/include/cpplib.h | 2 +- libcpp/include/line-map.h | 667 ++++++++++++++++++++++++++++++------------ libcpp/internal.h | 12 +- libcpp/line-map.c | 266 +++++++++-------- libcpp/location-example.txt | 216 ++++++++++++++ libcpp/macro.c | 18 +- 22 files changed, 1150 insertions(+), 364 deletions(-) create mode 100644 libcpp/location-example.txt -- 1.8.5.3