Re: [PATCH 1/2] gcc symbol database

[Dodji Seketeli <dodji at redhat dot com>]

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