Hello Yunfeng,
Thank you for following up, and sorry for me reviewing your patches so
lately. The libcpp changes are coming along nicely, IMHO. I like the
fact that they are getting pretty minimal. I just have a few mostly
cosmetic comments at this point.
[...]
> diff -cpr .pc/symdb_enhance_libcpp/libcpp/include/cpplib.h
> libcpp/include/cpplib.h
> *** .pc/symdb_enhance_libcpp/libcpp/include/cpplib.h Wed Jul 25 10:57:16 2012
> --- libcpp/include/cpplib.h Wed Jul 25 10:57:31 2012
> *************** struct cpp_callbacks
> *** 526,531 ****
> --- 526,548 ----
> be expanded. */
> cpp_hashnode * (*macro_to_expand) (cpp_reader *, const cpp_token *);
>
> + /* The more powerful function extracts token than cpp_get_token. Later
> + * callbacks show it. */
> + void (*lex_token) (cpp_reader *, const cpp_token*);
For the sake of clarity, I'd change the signature and comment to
something more classically informative like (note that there should be
two spaces after between the '.' and the */):
/* This callback is invoked when a token is lexed. The RESULT
parameter represents the lexed token. */
void (*lex_token) (cpp_reader *, const cpp_token *result);
> + /* The pair is called when gcc expands macro. Enable lex_token in
> + * macro_start_expand, you can catch all macro tokens. The pair can be
> called
> + * nested, and the second parameter of macro_end_expand notifies you
> whether
> + * macro is still expanding. */
> + void (*macro_start_expand) (cpp_reader *, const cpp_token *,
> + const cpp_hashnode *);
Likewise, I'd change this into something like what comes below. Also,
please try to use the same style as the rest of the comments of the
file; for instance, do not start each line of comment with a '*'; put
two spaces after each '.':
/* This callback is invoked whenever a function-like macro
represented by MACRO_NODE is about to be expanded. MACRO_TOKEN
is the token for the macro name and MACRO_NODE represents the
macro. */
void (*macro_start_expand) (cpp_reader *,
const cpp_token *macro_token,
const cpp_hashnode *macro_node);
> + void (*macro_end_expand) (cpp_reader *, bool);
I believe that you don't need the last parameter, because this
callback should be called only when we know the macro *has* been
expanded. So later down in _cpp_pop_context, I'd change:
*************** _cpp_pop_context (cpp_reader *pfile)
*** 2245,2250 ****
--- 2250,2257 ----
/* decrease peak memory consumption by feeing the context. */
pfile->context->next = NULL;
free (context);
+ if (pfile->cb.macro_end_expand)
+ pfile->cb.macro_end_expand (pfile, in_macro_expansion_p (pfile));
}
into:
if (in_macro_expansion_p (file) && pfile->cb.macro_end_expand)
pfile->cb.macro_end_expand (pfile);
The callback that you actually set in your application, in so.c (in
gcc.tgz) would then change from:
static void
cb_macro_end (cpp_reader * pfile, bool in_expansion)
{
cpp_callbacks *cb = cpp_get_callbacks (pfile);
if (!in_expansion)
{
cache_end_let ();
mo_leave ();
cb->lex_token = NULL;
cb->macro_end_expand = NULL;
}
}
to:
static void
cb_macro_end (cpp_reader * pfile)
{
cpp_callbacks *cb = cpp_get_callbacks (pfile);
cache_end_let ();
mo_leave ();
cb->lex_token = NULL;
cb->macro_end_expand = NULL;
}
And for the macro_end_expand, I'd add a comment and change the signature to:
/* This callback is invoked whenever the macro previously notified by
the macro_start_expand has been expanded. */
void (*macro_end_expand) (cpp_reader *);
> + /* The pair is called when cpp directive (starting from `#', such as
> + * `#define M', `#endif' etc) is encountered and reaches end. When enable
> + * lex_token in start_directive, the sequence is lex_token("define"),
> + * lex_token("M") ... */
> + void (*start_directive) (cpp_reader *);
I'd change the comment to:
/* This callback is invoked whenever a preprocessor directive (such as
#define M) is encountered and about to be handled. */
> + void (*end_directive) (cpp_reader *);
> +
And I'd add a comment here that roughly reads:
/* This callback is invoked when the directive previously notified
by a call to the start_directive callback has been handled.
Information about the directive is accessible at the field
PFILE->directive. */
void (*end_directive) (cpp_reader *pfile);
[...]
> static _cpp_buff *
> funlike_invocation_p (cpp_reader *pfile, cpp_hashnode *node,
> ! _cpp_buff **pragma_buff, unsigned *num_args)
> {
> const cpp_token *token, *padding = NULL;
>
> --- 963,970 ----
> returned buffer. */
> static _cpp_buff *
> funlike_invocation_p (cpp_reader *pfile, cpp_hashnode *node,
> ! _cpp_buff **pragma_buff, unsigned *num_args,
> ! const cpp_token *head)
> {
Please update the comment to say what the new parameter is. I'd call
it macro_token, instead of head.
[...]
I have only lightly looked at the C front-end changes. I'd like to
think about it more before sending comments, unless someone else beats
me to it.
Thanks.
--
Dodji
