This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH 1/7] Linemap infrastructure for virtual locations
On 08/09/2011 10:31 AM, Dodji Seketeli wrote:
-#define in_system_header (in_system_header_at (input_location))
+#define in_system_header (in_system_header_at (input_location))
Unnecessary whitespace change.
struct GTY(()) cpp_macro {
+ /* Name of this macro. Used only for error reporting. */
+ cpp_hashnode * GTY ((nested_ptr (union tree_node,
+ "%h ? CPP_HASHNODE (GCC_IDENT_TO_HT_IDENT (%h)) : NULL",
+ "%h ? HT_IDENT_TO_GCC_IDENT (HT_NODE (%h)) : NULL")))
+ name;
Would it make sense to have line_map_macro store a pointer to the
cpp_hashnode rather than straight to the cpp_macro so we don't have to
add an extra pointer to cpp_macro?
+struct GTY(()) cpp_macro;
I don't think we need GTY markers on forward declarations.
+ LC_ENTER_MACRO
+ /* stringize */
+ /* paste */
What is the purpose of these comments?
+ - when a new preprocessing unit start.
+ - when a preprocessing unit ends.
+ - when a macro expansion occurs
+ */
*/ should go at the end of the line.
+/*
+ Create a macro map. A macro map encodes source locations of tokens
And /* at the beginning of the line.
+linemap_check_ordinary (const struct line_map *map)
+{
+ linemap_assert (!linemap_macro_expansion_map_p (map));
+ /* Return any old value. */
+ return 0;
+}
Why does this return int if we aren't interested in the value? I
would change it to a macro that returns 'map' so that it can expand to
nothing if !ENABLE_CHECKING and can be used in-line like the gcc
TREE_CHECK macros.
... this. This is now implemented by a macro as you are suggesting.
You changed it to a macro that returns 'map' but didn't change the uses
to inline, i.e.
+ (linemap_check_ordinary (MAP), (MAP)->d.ordinary.to_file)
to
+ (linemap_check_ordinary (MAP)->d.ordinary.to_file)
For that to work, the !ENABLE_CHECKING version also needs to return MAP.
If you aren't going to change the users, the ENABLE_CHECKING version
might as well not return anything.
+typedef struct GTY (())
+{
The GTY(()) can't work here without a tag name associated with it. And
we never gc-allocate extended_location, so it isn't needed.
+enum location_resolution_kind
+{
+ LRK_MACRO_EXPANSION_POINT,
+ LRK_SPELLING_LOCATION,
+ LRK_MACRO_PARM_REPLACEMENT_POINT
+};
Let's move this after the comment that explains the values.
+
-static void trace_include (const struct line_maps *, const struct line_map *);
+static void trace_include (const struct line_maps *, const struct line_map *);
Unnecessary change.
+ /* If we don't keep our line maps consistent, we can easily
+ segfault. Don't rely on the client to do it for us. */
This sounds like working around a bug. Wouldn't it be better to fix it?
So this is a second try. I have removed that test altogether. But
then I figured I should nonetheless handle the case where we run out
of integer space when allocating locations for both ordinary and macro
tokens. So linemap_line_start hands out 0 in that case (for ordinary
tokens) and linemap_enter_macro hands out NULL in lieu of a macro map
in that case. That NULL map is handled (in the second patch of the
series) so that the spelling location of the macro token is used in
that case.
Is something still protecting from numeric overflow in the case that
there aren't any macros? I'd leave that test the way it was before (and
keep your new tests too).
+ /* If LOCATION is reserved for the user of libcpp, it means,
+ e.g. for gcc that it's the location of a built-in token. In that
+ case, let's say that the final location is the macro expansion
+ point because otherwise, the built-in location would not make
+ any sense and would violate the invariant that says that every
+ single location must be >= to the MAP_START_LOCATION (MAP) of its
+ map. */
How can this happen? Can't we just assert that it doesn't, like we do
in the next function?
Jason