Hello,
Similarly to a comment I made on the previous patch of the series,
+++ b/libcpp/include/line-map.h
[...]
struct GTY(()) location_adhoc_data {
source_location locus;
+ source_range src_range;
void * GTY((skip)) data;
};
Could we just change the type of locus and make it be a source_range
instead? With the right converting constructor (in the source_range
class) that takes a source_location, the amount of churn should
hopefully be minimized, or maybe I am missing something ...
[...]
diff --git a/libcpp/line-map.c b/libcpp/line-map.c
[...]
+source_range
+get_range_from_adhoc_loc (struct line_maps *set, source_location loc)
+{
Please add a comment for this function.
[...]
diff --git a/gcc/tree.c b/gcc/tree.c
+void
+set_source_range (tree *expr, location_t start, location_t finish)
Please add a comment for this function and its overloads.
[...]
+void
+set_c_expr_source_range (c_expr *expr,
+ location_t start, location_t finish)
+{
Likewise.
+location_t
+set_block (location_t loc, tree block)
+{
Likewise. I am wondering if we shouldn't even change the name of this
function to better suit what it does: associate a tree to a location.
+ source_range src_range;
+ if (IS_ADHOC_LOC (loc))
+ /* FIXME: can we update in-place? */
+ src_range = get_range_from_adhoc_loc (line_table, loc);
Hmmh, at the moment, I don't think we can update an entry of the adhoc
map that associates {location, range, tree} as all three components
contribute to the hash value of the entry. A new triplet means a new
entry.
My understanding is that initially the two elements of the pair
{location, tree} were contributing to the hash value because the same
location could very well belong to different blocks.
+ else
+ src_range = source_range::from_location (loc);
+
+ return COMBINE_LOCATION_DATA (line_table, loc, src_range, block);
+}
[...]
@@ -6091,6 +6112,10 @@ c_parser_conditional_expression (c_parser *parser,
struct c_expr *after,
if (c_parser_next_token_is_not (parser, CPP_QUERY))
return cond;
+ if (cond.value != error_mark_node)
+ start = cond.src_range.m_start;
+ else
+ start = UNKNOWN_LOCATION;
I think that "getting the start range of a c_expr" is an operation
that is generally useful; and it's also generally useful for that
operation to handle cases where the tree carried by the c_expr can be
an error_mark_node. So maybe it would be appropriate to have a getter
function for that operation and use it here instead.
You would then use that operation in the other places of this patch
that are getting c_expr::src_range.start -- by the way, those other
places are not handling the error_mark_node case like the above.
[...]
+++ b/gcc/c/c-tree.h
@@ -132,6 +132,9 @@ struct c_expr
The type of an enum constant is a plain integer type, but this
field will be the enum type. */
tree original_type;
+
+ /* FIXME. */
+ source_range src_range;
};
Why a FIXME here?
+#define CAN_HAVE_RANGE_P(NODE) (CAN_HAVE_LOCATION_P (NODE))
Please add a comment for this.
+#define EXPR_LOCATION_RANGE(NODE) (get_expr_source_range (EXPR_CHECK ((NODE))))
Likewise.
+#define EXPR_HAS_RANGE(NODE) \
+ (CAN_HAVE_RANGE_P (NODE) \
+ ? EXPR_LOCATION_RANGE (NODE).m_start != UNKNOWN_LOCATION \
+ : false)
+
Likewise.
[...]
+static inline source_range
+get_expr_source_range (tree expr)
Likewise.
+#define DECL_LOCATION_RANGE(NODE) \
+ (get_decl_source_range (DECL_MINIMAL_CHECK (NODE)))
+
Likewise.
+static inline source_range
+get_decl_source_range (tree decl)
+{
Likewise.
--
Dodji
