Re: [PATCH 1/2] gcc symbol database

[Dodji Seketeli <dodji at redhat dot com>]

Hello YunFeng,

Thank you for taking the time to work on this.  I cannot accept or
deny your patches, but I thought I could maybe comment on some parts
of them.

First, IMHO, some of the new plugin events you are proposing to add to
libcpp seem to make sense, at least so that people can write
etags/ctags-like cross-reference tools like the one you are proposing.

For now, I will not comment on the choices you've made for the
cross-reference tool itself, even though they are interesting to see,
at least to understand why you need the libcpp events you are
proposing.

Thus, I'll start with the libcpp changes.

[...]

> diff -upr .pc/symdb_enhance_libcpp/libcpp/directives.c libcpp/directives.c

[...]

> @@ -492,6 +492,8 @@ _cpp_handle_directive (cpp_reader *pfile
>    else if (skip == 0)
>      _cpp_backup_tokens (pfile, 1);
> 
> +  if (pfile->cb.end_directive)
> +    pfile->cb.end_directive (pfile);

It seems to me that there are possibly several places where we handle
directives.  In those places, the function start_directive and
end_directive are called.  So IMHO, it would seems more appropriate
and maintainable to call pfile->cb.start_directive in the
start_directive function, and likewise, call pfile->cg.end_directive
in the end_directive function.

You would then remove the call below from _cpp_lex_token:

@@ -1886,6 +1889,8 @@ _cpp_lex_token (cpp_reader *pfile)
          handles the directive as normal.  */
           && pfile->state.parsing_args != 1)
         {
+          if (pfile->cb.start_directive)
+        pfile->cb.start_directive (pfile, result);
           if (_cpp_handle_directive (pfile, result->flags & PREV_WHITE))
         {
           if (pfile->directive_result.type == CPP_PADDING)

[...]


> +++ libcpp/include/cpplib.h    2012-05-25 14:56:56.745507332 +0800
> @@ -218,10 +218,10 @@ struct GTY(()) cpp_identifier {
>         node;
>  };
> 
> -/* A preprocessing token.  This has been carefully packed and should
> -   occupy 16 bytes on 32-bit hosts and 24 bytes on 64-bit hosts.  */
> +/* A preprocessing token. */
>  struct GTY(()) cpp_token {
>    source_location src_loc;    /* Location of first char of token.  */
> +  int file_offset;

Enlarging this struct that is *widely* used might not be accepted by
the maintainers, because of the memory toll it takes.  Or you'd need
to prove that you cannot do otherwise.

So why is this file_offset member needed?

>From the rest of the patch, it looks like you are using it to store
the offset of the token, from the beginning of its line.  What is the
difference between that, and the column number of the token, as you
could get by calling linemap_expand_location on the src_loc of the
token, effectively doing away with this new file_offset member?  Note
that the linemap_expand_location call was introduced in 4.7.

[...]

> +++ libcpp/macro.c    2012-05-25 14:56:56.749508416 +0800
> @@ -1029,9 +1029,13 @@ enter_macro_context (cpp_reader *pfile,
>            if (pragma_buff)
>          _cpp_release_buff (pfile, pragma_buff);
> 
> +          if (pfile->cb.macro_end_arg)
> +        pfile->cb.macro_end_arg (pfile, true);

Presumably, if we reach this point, I believe it means there was no
argument to what seemed to be a function-like macro.  Was it?

Thus, I don't understand why calling this pfile->cb.macro_end_arg call
achieves.  Actually, I think I don't understand the meaning of that
event callback, to begin with.  I have read and re-read the comment
below:

> @@ -522,6 +522,24 @@ struct cpp_callbacks
>       be expanded.  */

[...]

> +  /* Called when a function-like macro stops collecting macro parameters,
> +   * cancel = true, macro expansion is canceled. */
> +  void (*macro_end_arg) (cpp_reader *, bool cancel);

and I don't understand why you need the information conveyed by this
macro_end_arg event.

Maybe if you put the call to macro_start_expand in this
enter_macro_context function only after we are sure we are going to
actually proceed with the expansion ....

>            return 0;
>          }

... here, then you wouldn't need the macro_end_arg at all.  Would that
make sense?

>        if (macro->paramc > 0)
>          replace_args (pfile, node, macro,
>                (macro_arg *) buff->base,
> @@ -2263,6 +2267,8 @@ cpp_get_token_1 (cpp_reader *pfile, sour
>        if (pfile->context->c.macro)
>          ++num_expanded_macros_counter;
>        _cpp_pop_context (pfile);
> +      if (pfile->cb.macro_end_expand)
> +        pfile->cb.macro_end_expand (pfile);

_cpp_pop_context is really the function that marks the end of a given
macro expansion, especially when the predicate in_macro_expansion_p
(introduced recently in trunk for gcc 4.8) returns true.

So IMHO, I seems more maintainable to move the call to
pfile->cb.macro_end_expand in _cpp_pop_context.

>        if (pfile->state.in_directive)
>          continue;
>        result = &pfile->avoid_paste;
> @@ -2321,8 +2327,14 @@ cpp_get_token_1 (cpp_reader *pfile, sour
>          }
>          }
>        else
> -        ret = enter_macro_context (pfile, node, result,
> -                       virt_loc);
> +          {
> +           if (pfile->cb.macro_start_expand)
> +         pfile->cb.macro_start_expand (pfile, result, node);
> +       ret = enter_macro_context (pfile, node, result, virt_loc);

IMHO, it's more maintainable to put the call to
pfile->cb.macro_start_expand inside enter_macro_context, as that is
the function that really triggers the macro expansion.

> +     if (ret == 0 && pfile->cb.macro_end_expand)
> +       /* macro expansion is canceled. */
> +       pfile->cb.macro_end_expand (pfile);
> +         }

Note that if you move the call to pfile->cb.macro_end_expand into the
_cpp_pop_context function as suggested earlier, you can just remove it
from here.

Thank you.

-- 
		Dodji