Yunfeng ZHANG <zyf.zer...@gmail.com> writes:

>> But the "meaning" of the macro_end_arg event is really not clear to
>> ...
>> .... and so on.
>
> Let's see a sample:
>     #define Z(a) a
>     #define Y Z
>     #define X(p) p + Y
>     X(1)(2);
> With my solution, user get
>     1) `X' -- leader macro token by macro_start_expand.
>     2) `(', `1', `)', `(', `2', `)' -- macro tokens, by cb_lex_token.
>     3) macro_end_arg.
>     4) `1', `+', `2' -- macro replacement tokens, by symdb_cpp_token.
>     5) macro_end_expand.

I see.  So the meaning of macro_end_arg is something like:

    "The tokens that were just reported by the lex_token event were
     arguments to a function-like macro".

Is that correct?  

If yes, then I guess you can keep that macro_end_arg event, but then I'd
remove its "cancel" argument by doing what I said in my previous
messages.  Which is, don't call macro_start_expand if X is not going to
be expanded.  To do that, move macro_start_expand in
enter_macro_context, like I proposed there.

Of course, that would not change the sequence of the events you'd get in
the example above.  But in this example below:

     1      #define Y 1
     2  
     3      int
     4      Y (int a)
     5      {
     6        return a + 1;
     7      }
     8  
     9      int
    10      main ()
    11      {
    12        return Y(a);
    13      }

When parsing Y(a), what I am proposing won't emit a
macro_start_expansion at line 12, but with your implementation, it will
emit a macro_start_expansion, followed by a macro_end_arg with the
cancel argument set to true.  Which is unnecessary if the
macro_start_expansion is not called at all there.

Of course, you'd have to change the hunk:

    *************** enter_macro_context (cpp_reader *pfile, 
    *** 1015,1020 ****
    --- 1015,1022 ----
              pfile->state.parsing_args = 1;
              buff = funlike_invocation_p (pfile, node, &pragma_buff,
                                           &num_args);
    +     if (pfile->cb.macro_end_arg)
    +       pfile->cb.macro_end_arg (pfile, buff == NULL);
              pfile->state.parsing_args = 0;
              pfile->keep_tokens--;
              pfile->state.prevent_expansion--;

so that "pfile->cb.macro_end_arg (pfile)" is not invoked when
funlike_invocation_p returns NULL.

Does that make sense?

If yes, the comment of that event should be adapted to have a more
meaningful meaning.  I believe the comments of the other events should
be adapted to, in the light of the description that I did in my previous
message.

>> Please reply to these questions so that I understand why you need this
>> field at all, rather than getting it from the linemap infrastructure
>> by calling, for instance, linemap_expand_location.
>
> linemap_expand_location can return the column and row of a token, not file
> offset of its, there's no more explanation why using token file-offset, when I
> started my project, I think file-offset is better than current
> linemap+source_location because it costed less time to encode/decode
> source_location field and can act just like previous solution,

What I meant was that if you prefer storing file offsets in the database
of your plugin, rather than line/column, then why not saying that it's
equal to something like (line + column), instead of adding a new field
in libcpp itself.

> it's an improvement to gcc too, with it gcc can store
> symbol+fileoffset to elf intern for ld/gdb usage, of course,

I wouldn't be so sure.  DWARF already has a sophisticated and compact
way to store line/column information, so I don't think the file offset
gives us anything in practice.

> my database can benefit from it -- less space and use it as sort field
> directly.

Sure, but then you can just compute it from the result of
linemap_expand_location, as I said above.

> The only thing is it make challenge on gcc infrastructure, so I leave
> it to a seperate patch called gcc_negotiate_patch and hope to discuss
> the first two patches only.

If you are not convinced, then OK, let's leave that part aside from now.

> And to the field file-offset, when the token is macro-replacement token, I
> recommend token.file_offset = -1 * <leader macro token>.file_offset. I think
> gdb is happy to this.

Right now, for tokens resulting from macro expansion, the location that
is encoded in debug information is already the location of the expansion
point of the macro.  So we already do what you say, in a sense.

Cheers.

-- 
                Dodji

Reply via email to