From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 60739 invoked by alias); 4 May 2015 20:45:59 -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 60727 invoked by uid 89); 4 May 2015 20:45:59 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.1 required=5.0 tests=AWL,BAYES_50,KAM_ASCII_DIVIDERS,SPF_HELO_PASS,SPF_PASS,T_RP_MATCHES_RCVD autolearn=no 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; Mon, 04 May 2015 20:45:56 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t44KjtQe013924 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Mon, 4 May 2015 16:45:55 -0400 Received: from localhost.localdomain (ovpn-113-143.phx2.redhat.com [10.3.113.143]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t44Kjsfn029059; Mon, 4 May 2015 16:45:54 -0400 Message-ID: <5547DA82.7000908@redhat.com> Date: Mon, 04 May 2015 20: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 , gcc-patches@gcc.gnu.org Subject: Re: [PATCH 4/4] Replace line_map union with C++ class hierarchy References: <1430528215-19648-1-git-send-email-dmalcolm@redhat.com> <1430528215-19648-5-git-send-email-dmalcolm@redhat.com> In-Reply-To: <1430528215-19648-5-git-send-email-dmalcolm@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2015-05/txt/msg00256.txt.bz2 On 05/01/2015 06:56 PM, David Malcolm wrote: > This patch eliminates the union in struct line_map in favor of > a simple class hierarchy, making struct line_map a base class, > with line_map_ordinary and line_map_macro subclasses. > > The patch eliminates all usage of linemap_check_ordinary and > linemap_check_macro from line-map.h, updating return types and > signatures throughout libcpp and gcc's usage of it to use the > appropriate subclasses. > > This moves the checking of linemap kind from run-time to > compile-time, and also implicitly documents everywhere where > the code is expecting an ordinary map vs a macro map vs > either kind of map. I believe it makes the code significantly > simpler: most of the accessor functions in line-map.h become > trivial field-lookups. > > I attemped to use templates for maps_info, but was stymied by > gengtype, so in the end I simply split it manually into > maps_info_ordinary and maps_info_macro. In theory it's just > a vec<>, but vec.h is in gcc, and thus not available > for use from libcpp. > > In a similar vein, gcc/is-a.h is presumably not usable > from within libcpp. If it were, there would be the following > rough equivalences: > > --------------------------------- -------------------------------- > line-map.h is-a.h > --------------------------------- -------------------------------- > linemap_check_ordinary (m) as_a (m) > linemap_check_macro (m) as_a (m) > linemap_macro_expansion_map_p (m) (M ? is_a (m) > : false) > --------------------------------- -------------------------------- > > There are numerous places in libcpp that offset a > line_map * using array notation to get the next/prev line_map of the > same kind, e.g.: > MAP_START_LOCATION (&cached[1]) > which breaks due to the different sizes of line_map vs its subclasses. > > On x86_64 host, before: > (gdb) p sizeof(line_map) > $1 = 40 > > after: > (gdb) p sizeof(line_map) > $1 = 8 > (gdb) p sizeof(line_map_ordinary) > $2 = 32 > (gdb) p sizeof(line_map_macro) > $3 = 40 > > Tracking down all of these array-based offsets to use a pointer to the > appropriate subclass (and thus use the correct offset) was rather > involved, but I believe the patch fixes them all now. > > (the patch thus also gives a very modest saving of 8 bytes per ordinary > line map). > > I've tried to use the naming convention "ord_map" and "macro_map" > whenever the typesystem ensures we're dealing with such a map, > wherever this is doable without needing to touch lines of code that > would otherwise not need touching by the patch. > > gcc/ChangeLog: > * diagnostic.c (diagnostic_report_current_module): Strengthen > local "new_map" from const line_map * to > const line_map_ordinary *. > * genmatch.c (error_cb): Likewise for local "map". > (output_line_directive): Likewise for local "map". > * input.c (expand_location_1): Likewise for local "map". > Pass NULL rather than &map to > linemap_unwind_to_first_non_reserved_loc, since the value is never > read from there, and the value written back not read from here. > (is_location_from_builtin_token): Strengthen local "map" from > const line_map * to const line_map_ordinary *. > (dump_location_info): Strengthen locals "map" from > line_map *, one to const line_map_ordinary *, the other > to const line_map_macro *. > * tree-diagnostic.c (loc_map_pair): Strengthen field "map" from > const line_map * to const line_map_macro *. > (maybe_unwind_expanded_macro_loc): Add a call to > linemap_check_macro when writing to the "map" field of the > loc_map_pair. > Introduce local const line_map_ordinary * "ord_map", using it in > place of "map" in the part of the function where we know we have > an ordinary map. Strengthen local "m" from const line_map * to > const line_map_ordinary *. > > gcc/ada/ChangeLog: > * gcc-interface/trans.c (Sloc_to_locus1): Strenghthen local "map" > from line_map * to line_map_ordinary *. > > gcc/c-family/ChangeLog: > * c-common.h (fe_file_change): Strengthen param from > const line_map * to const line_map_ordinary *. > (pp_file_change): Likewise. > * c-lex.c (fe_file_change): Likewise. > (cb_define): Use linemap_check_ordinary when invoking > SOURCE_LINE. > (cb_undef): Likewise. > * c-opts.c (c_finish_options): Use linemap_check_ordinary when > invoking cb_file_change. > (c_finish_options): Likewise. > (push_command_line_include): Likewise. > (cb_file_change): Strengthen param "new_map" from > const line_map * to const line_map_ordinary *. > * c-ppoutput.c (cb_define): Likewise for local "map". > (pp_file_change): Likewise for param "map" and local "from". > > gcc/fortran/ChangeLog: > * cpp.c (maybe_print_line): Strengthen local "map" from > const line_map * to const line_map_ordinary *. > (cb_file_change): Likewise for param "map" and local "from". > (cb_line_change): Likewise for local "map". > > libcpp/ChangeLog: > * directives.c (do_line): Strengthen local "map" from > const line_map * to const line_map_ordinary *. > (do_linemarker): Likewise. > (_cpp_do_file_change): Assert that we're not dealing with > a macro map. Introduce local "ord_map" via a call to > linemap_check_ordinary, guarded within the check for > non-NULL. Use it for typesafety. > * files.c (cpp_make_system_header): Strengthen local "map" from > const line_map * to const line_map_ordinary *. > * include/cpplib.h (struct cpp_callbacks): Likewise for second > parameter of "file_change" callback. > * include/line-map.h (struct line_map): Convert from a struct > containing a union to a base class. > (struct line_map_ordinary): Convert to a subclass of line_map. > (struct line_map_macro): Likewise. > (linemap_check_ordinary): Strengthen return type from line_map * > to line_map_ordinary *, and add a const-variant. > (linemap_check_macro): New pair of functions. > (ORDINARY_MAP_STARTING_LINE_NUMBER): Strengthen param from > const line_map * to const line_map_ordinary *, eliminating call > to linemap_check_ordinary. Likewise for the non-const variant. > (ORDINARY_MAP_INCLUDER_FILE_INDEX): Likewise. > (ORDINARY_MAP_IN_SYSTEM_HEADER_P): Likewise. > (ORDINARY_MAP_NUMBER_OF_COLUMN_BITS): Likewise. > (ORDINARY_MAP_FILE_NAME): Likewise. > (MACRO_MAP_MACRO): Strengthen param from const line_map * to > const line_map_macro *. Likewise for the non-const variant. > (MACRO_MAP_NUM_MACRO_TOKENS): Likewise. > (MACRO_MAP_LOCATIONS): Likewise. > (MACRO_MAP_EXPANSION_POINT_LOCATION): Likewise. > (struct maps_info): Replace with... > (struct maps_info_ordinary):...this and... > (struct maps_info_macro): ...this. > (struct line_maps): Convert fields "info_ordinary" and > "info_macro" to the above new structs. > (LINEMAPS_MAP_INFO): Delete both functions. > (LINEMAPS_MAPS): Likewise. > (LINEMAPS_ALLOCATED): Rewrite both variants to avoid using > LINEMAPS_MAP_INFO. > (LINEMAPS_USED): Likewise. > (LINEMAPS_CACHE): Likewise. > (LINEMAPS_MAP_AT): Likewise. > (LINEMAPS_ORDINARY_MAPS): Strengthen return type from line_map * > to line_map_ordinary *. > (LINEMAPS_ORDINARY_MAP_AT): Likewise. > (LINEMAPS_LAST_ORDINARY_MAP): Likewise. > (LINEMAPS_LAST_ALLOCATED_ORDINARY_MAP): Likewise. > (LINEMAPS_MACRO_MAPS): Strengthen return type from line_map * to > line_map_macro *. > (LINEMAPS_MACRO_MAP_AT): Likewise. > (LINEMAPS_LAST_MACRO_MAP): Likewise. > (LINEMAPS_LAST_ALLOCATED_MACRO_MAP): Likewise. > (linemap_map_get_macro_name): Strengthen param from > const line_map * to const line_map_macro *. > (SOURCE_LINE): Strengthen first param from const line_map * to > const line_map_ordinary *, removing call to > linemap_check_ordinary. > (SOURCE_COLUMN): Likewise. > (LAST_SOURCE_LINE_LOCATION): Likewise. > (LAST_SOURCE_LINE): Strengthen first param from const line_map * > to const line_map_ordinary *. > (LAST_SOURCE_COLUMN): Likewise. > (INCLUDED_FROM): Strengthen return type from line_map * to > line_map_ordinary *., and second param from const line_map * > to const line_map_ordinary *, removing call to > linemap_check_ordinary. > (MAIN_FILE_P): Strengthen param from const line_map * to > const line_map_ordinary *, removing call to > linemap_check_ordinary. > (linemap_position_for_line_and_column): Strengthen param from > const line_map * to const line_map_ordinary *. > (LINEMAP_FILE): Strengthen param from const line_map * to > const line_map_ordinary *, removing call to > linemap_check_ordinary. > (LINEMAP_LINE): Likewise. > (LINEMAP_SYSP): Likewise. > (linemap_resolve_location): Strengthen final param from > const line_map ** to const line_map_ordinary **. > * internal.h (CPP_INCREMENT_LINE): Likewise for local "map". > (linemap_enter_macro): Strengthen return type from > const line_map * to const line_map_macro *. > (linemap_add_macro_token): Likewise for first param. > * line-map.c (linemap_check_files_exited): Strengthen local "map" > from const line_map * to const line_map_ordinary *. > (new_linemap): Introduce local "map_size" and use it when > calculating how large the buffer should be. Rewrite based > on change of info_macro and info_ordinary into distinct types. > (linemap_add): Strengthen locals "map" and "from" from line_map * > to line_map_ordinary *. > (linemap_enter_macro): Strengthen return type from > const line_map * to const line_map_macro *, and local "map" from > line_map * to line_map_macro *. > (linemap_add_macro_token): Strengthen param "map" from > const line_map * to const line_map_macro *. > (linemap_line_start): Strengthen local "map" from line_map * to > line_map_ordinary *. > (linemap_position_for_column): Likewise. > (linemap_position_for_line_and_column): Strengthen first param > from const line_map * to const line_map_ordinary *. > (linemap_position_for_loc_and_offset): Strengthen local "map" from > const line_map * to const line_map_ordinary *. > (linemap_ordinary_map_lookup): Likewise for return type and locals > "cached" and "result". > (linemap_macro_map_lookup): Strengthen return type and locals > "cached" and "result" from const line_map * to > const line_map_macro *. > (linemap_macro_map_loc_to_exp_point): Likewise for param "map". > (linemap_macro_map_loc_to_def_point): Likewise. > (linemap_macro_map_loc_unwind_toward_spelling): Likewise. > (linemap_get_expansion_line): Strengthen local "map" from > const line_map * to const line_map_ordinary *. > (linemap_get_expansion_filename): Likewise. > (linemap_map_get_macro_name): Strengthen param from > const line_map * to const line_map_macro *. > (linemap_location_in_system_header_p): Add call to > linemap_check_ordinary in region guarded by > !linemap_macro_expansion_map_p. Introduce local "macro_map" via > linemap_check_macro in other region, using it in place of "map" > for typesafety. > (first_map_in_common_1): Add calls to linemap_check_macro. > (trace_include): Strengthen param "map" from const line_map * to > const line_map_ordinary *. > (linemap_macro_loc_to_spelling_point): Strengthen final param from > const line_map ** to const line_map_ordinary **. Replace a > C-style cast with a const_cast, and add calls to > linemap_check_macro and linemap_check_ordinary. > (linemap_macro_loc_to_def_point): Likewise. > (linemap_macro_loc_to_exp_point): Likewise. > (linemap_resolve_location): Strengthen final param from > const line_map ** to const line_map_ordinary **. > (linemap_unwind_toward_expansion): Introduce local "macro_map" via > a checked cast and use it in place of *map. > (linemap_unwind_to_first_non_reserved_loc): Strengthen local > "map1" from const line_map * to const line_map_ordinary *. > (linemap_expand_location): Introduce local "ord_map" via a checked > cast and use it in place of map. > (linemap_dump): Make local "map" const. Strengthen local > "includer_map" from line_map * to const line_map_ordinary *. > Introduce locals "ord_map" and "macro_map" via checked casts and > use them in place of "map" for typesafety. > (linemap_dump_location): Strengthen local "map" from > const line_map * to const line_map_ordinary *. > (linemap_get_file_highest_location): Update for elimination of > union. > (linemap_get_statistics): Strengthen local "cur_map" from > line_map * to const line_map_macro *. Update uses of sizeof to > use the appropriate line_map subclasses. > * macro.c (_cpp_warn_if_unused_macro): Add call to > linemap_check_ordinary. > (builtin_macro): Strengthen local "map" from const line_map * to > const line_map_macro *. > (enter_macro_context): Likewise. > (replace_args): Likewise. > (tokens_buff_put_token_to): Likewise for param "map". > (tokens_buff_add_token): Likewise. > --- > @@ -158,7 +158,7 @@ expand_location_1 (source_location loc, > location (toward the expansion point) that is not reserved; > that is, the first location that is in real source code. */ > loc = linemap_unwind_to_first_non_reserved_loc (line_table, > - loc, &map); > + loc, NULL); Is that really correct? /* Return the map at a given index. */ > inline line_map * > LINEMAPS_MAP_AT (const line_maps *set, bool map_kind, int index) > { > - return &(LINEMAPS_MAPS (set, map_kind)[index]); > + if (map_kind) > + return &set->info_macro.maps[index]; > + else > + return &set->info_ordinary.maps[index]; > } I see this pattern repeated regularly. Any thoughts on how painful it'll be to drop the map_kind argument and instead not vary the kind? I just spot checked all the mechanical changes. Looks reasonable me to me. Just need to sort out if the hunk with the call to l_u_t_f_n_r_l is correct and get the pre-reqs installed and I think this is good to go. Jeff