Hello Dehao,
I have mostly cosmetic comments to make about the libcpp parts.
Dehao Chen <de...@google.com> writes:
> Index: libcpp/include/line-map.h
> ===================================================================
> *** libcpp/include/line-map.h (revision 189835)
> --- libcpp/include/line-map.h (working copy)
> *************** struct GTY(()) line_map_ordinary {
> *** 89,95 ****
>
Just to sum things up, here is what I understand from the libcpp
changes.
The integer space is now segmented like this:
X A B C
[ ] <--- integer space of source_location
C is the smallest possible source_location and X is the biggest
possible.
>From X to A, we have instances of source_location encoded in an ad-hoc
client specific (from the point of view of libcpp/line-map) map.
>From A to B, we have instances of source_location encoded in macro maps.
>From B to C, we have instances of source_location encoded in ordinary
maps.
As of this patch, MAX_SOURCE_LOCATION is A. X is 0xFFFFFFFF and is not
represented by any particular constant declaration.
>From the point of view libcpp the goal of this patch is to
1/ Provide functions to encode a pair of
{non-ad-hoc source_location, client specific data} and yield an
integer for that (a new instance of source_location).
2/ modify the lookup routines of line-map to have them recognize
instances of source_location encoded in the ad-hoc map.
If this description is correct, I think a high level comment like this
should be added to line-map.c or line-map.h to help people understand
this in the future, a bit like what has been done for ordinary and macro
maps.
[...]
> + extern void *get_block_from_location (source_location);
I'd call this function get_block_from_ad_hoc_location instead. Or
something like that, to hint at the fact that the parameter is not an
actual source location.
> + extern source_location get_locus_from_location (source_location);
Likewise, I'd call this get_source_location_from_ad_hoc_loc.
> +
> + #define COMBINE_LOCATION(LOC, BLOCK) \
> + ((BLOCK) ? get_combine_location ((LOC), (BLOCK)) : (LOC))
> + #define IS_COMBINED_LOCATION(LOC) (((LOC) & MAX_SOURCE_LOCATION) != (LOC))
> +
> /* Initialize a line map set. */
> extern void linemap_init (struct line_maps *);
>
> *************** typedef struct
> *** 594,599 ****
> --- 604,611 ----
>
> int column;
>
> + void *block;
> +
I'd just call this 'data' or something like, and add I comment
explaining that its a client specific data that is not manipulated by
libcpp.
> /* In a system header?. */
> bool sysp;
> } expanded_location;
> --- libcpp/line-map.c (working copy)
[...]
> + struct location_block {
> + source_location locus;
> + void *block;
> + };
I think we should have a more general name than "block" here. I am
thinking that other client code might be willing to associate entities
other than blocks to a given source_location in a scheme similar to this
one. Also, it seems surprising to have the line-maps library deal with
blocks specifically.
So maybe something like this instead?:
/* Data structure to associate an arbitrary data to a source location. */
struct location_ad_hoc_data {
source_location locus;
void *data;
};
Subsequently, if you agree with this, all the occurrences of 'block' in
the new code (function, types, and variable names) you introduced should
be replaced with 'ad_hoc_data'.
> + source_location
> + get_combine_location (source_location locus, void *block)
Shouldn't this be get_combined_location instead? Note the 'd' after the
combine.
> --- gcc/input.h (working copy)
[...]
> + #define LOCATION_LOCUS(LOC) \
> + ((IS_COMBINED_LOCATION(LOC)) ? get_locus_from_location (LOC) : (LOC))
I think this name sounds confusing. Maybe something like
SOURCE_LOCATION_FROM_AD_HOC_LOC instead? And please add a comment to
explain a little bit about this business of ad hoc location that
associates a block with an actual source location.
> + #define LOCATION_BLOCK(LOC) \
> + ((tree) ((IS_COMBINED_LOCATION (LOC)) ? get_block_from_location (LOC) \
> + : NULL))
And the name of this macro would then be something like BLOCK_FROM_AD_HOC_LOC.
> + #define IS_UNKNOWN_LOCATION(LOC) \
> + ((IS_COMBINED_LOCATION (LOC)) ? get_locus_from_location (LOC) == 0 \
> + : (LOC) == 0)
--
Dodji
