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