Re: make grokfield handle locations

["Manuel López-Ibáñez" <lopezibanez at gmail dot com>]

2008/8/19 Aldy Hernandez <aldyh@redhat.com>:
> OK for mainline?

I am not a maintainer, I cannot approve or reject patches.

> There is no testcase because it's already here: gcc.dg/20011008-1.c.
> But since -fno-show-column is passed to dg/ tests, it goes untested.

You can enable column numbers explicitly with dg-options
"-fshow-column". It will (*should*) override any global settings. Then
you can test by doing:

dg-error "11: error"

which is cumbersome and ugly but is what we have now. And it is going
to grow unless someone fixes the testsuite to handle column numbers in
a nicer way.

> As a follow up patch, I would like to make the dg/ machinery understand
> columns.  I am open to suggestions, but I think the easiest approach
> would be to have the dg-{warning,error} start on the expected column on
> the the following line-- like this:
>
>        struct { int x; int; } a;
>                           /* { dg-warning "does not declare anything" }

I think that would really hard. You should ask Janis for advice but
AFAIK dejagnu does not have any idea at which column the comment
starts.

I guess that changing the way dejagnu formats line numbers (it
currently uses %d to create the match, see dg.exp in your dejagnu
installation), the following could be easily implemented:

/* { dg-warning "does not declare anything" "" { target *-*-* } "5:11" }

If that is not possible, adding an explicit optional column number
should not be too hard:

/* { dg-warning "does not declare anything" "" { target *-*-* } "5" "11" }

But again, better ask Janis first.

> What do y'all think?

I think that in principle we want to do this. We want to mostly get
rid of input_location. In the future, we may decide to embed some
locations within the parser structures, so we do not need to
explicitly pass them around all the time. But for the time being,
passing them around will give us a clearer picture of what is
available where and what is needed.

> @@ -2002,14 +2004,17 @@ c_parser_struct_declaration (c_parser *p
>          tree postfix_attrs = NULL_TREE;
>          tree width = NULL_TREE;
>          tree d;
> -         if (c_parser_next_token_is (parser, CPP_COLON))
> +         c_token *tok = c_parser_peek_token (parser);
> +
> +         if (tok->type == CPP_COLON)
>            {
>              c_parser_consume_token (parser);
>              width = c_parser_expr_no_commas (parser, NULL).value;
>            }

I think this is not correct. Between this line and the next one, the
next token may have changed and the location pointed by tok would be
not valid anymore.

> -         if (c_parser_next_token_is_keyword (parser, RID_ATTRIBUTE))
> +         if (tok->keyword == RID_ATTRIBUTE)
>            postfix_attrs = c_parser_attributes (parser);

Again, we may have run out of tokens between this and the next line,
so tok may not be referring to the current token in the parser. I
think that if you want to save a specific location earlier, then you
should do it explicitly instead of saving the token. If you want to
pass the current token's location, then you should use:

grokfiled (c_parser_peek_token (parser)->location,

Optimization should take care of getting rid of redundant checks.


> -         d = grokfield (declarator, specs, width, &all_prefix_attrs);
> +         d = grokfield (tok->location,
> +                        declarator, specs, width, &all_prefix_attrs);


Cheers,

Manuel.