To Dodji Seketeli:
Thanks, I will republish my libcpp patch later.
2012/6/4 Dodji Seketeli <do...@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
