Re: [PATCH 1/2] gcc symbol database

[Yunfeng ZHANG <zyf dot zeroos at gmail dot com>]

To Dodji Seketeli:
Thanks, I will republish my libcpp patch later.

2012/6/4 Dodji Seketeli <dodji@redhat.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