Re: [PATCH 2/7] Generate virtual locations for tokens

[Jason Merrill <jason@redhat.com>]

On 09/14/2011 04:50 AM, Dodji Seketeli wrote:
> To comply with this invariant, the initial patch of Tom was cloning T
> into T', and was using T'->src_loc to track the virtual location of T.
> Which is close to what you are proposing, while respecting the
> invariant.

Yes, that's what I had in mind.  I didn't realize that there was only 
one token for FOO.

> But it turned out that was using too much memory :-(.  So
> we devised this scheme instead.

Ah. :(

> +  void *macro;

This should be a union rather than an untyped pointer.

> +      else if (context->tokens_kind == TOKENS_KIND_EXTENDED)
> +       {
> +         /* So we are in presence of an extended token context, which
> +            means that each token in this context has a virtual
> +            location attached to it.  So let's not forget to update
> +            the pointer to the current virtual location of the
> +            current token when we update the pointer to the current
> +            token */
> +
> +         rhs = *FIRST (context).ptoken++;
> +         if (context->macro)

The other places that deal with TOKENS_KIND_EXTENDED don't test that 
context->macro is non-null.  Why is it needed here?

> +    {
> +      cpp_hashnode *macro;
> +      if (context->tokens_kind == TOKENS_KIND_EXTENDED)
> +       {
> +         macro_context *mc = (macro_context *) context->macro;
> +         macro = mc->macro_node;
> +         /* If context->buff is set, it means the life time of tokens
> +            is bound to the life time of this context; so we must
> +            free the tokens; that means we must free the virtual
> +            locations of these tokens too.  */
> +         if (context->buff && mc->virt_locs)
> +           {
> +             free (mc->virt_locs);
> +             mc->virt_locs = NULL;
> +           }
> +         free (mc);
> +         context->macro = NULL;
> +       }
> +      else
> +       macro = (cpp_hashnode *) context->macro;
> +
> +      if (macro != NULL)
> +       macro->flags &= ~NODE_DISABLED;

How can macro end up NULL if context->macro was set?

> +/* In the traditionnal mode of the preprocessor, if we are currently
> +   location if we are in the traditionnal mode, and just returns

"traditional"

I don't think we need to talk about virtual locations before 
cpp_get_token_1; it's not an external interface, and it's redundant with 
the description before cpp_get_token_with_location.

> +  result = cpp_get_token_1 (pfile, loc);
>    if (pfile->context->macro)
> -    *loc = pfile->invocation_location;
> +    {
> +      if (!CPP_OPTION (pfile, track_macro_expansion))
> +       *loc = pfile->invocation_location;
> +    }
>    else
>      *loc = result->src_loc;
>
> +  *loc = maybe_adjust_loc_for_trad_cpp (pfile, *loc);

Let's move this code into cpp_get_token_1 so that all the location 
tweaking is in one place.

> +  switch (it->kind)
> +    {
> +    case MACRO_ARG_TOKEN_NORMAL:
> +    case MACRO_ARG_TOKEN_EXPANDED:
> +      it->token_ptr++;
> +      if (track_macro_exp_p)
> +       it->location_ptr++;
> +      break;
> +    case MACRO_ARG_TOKEN_STRINGIFIED:
> +#ifdef ENABLE_CHECKING
> +      if (it->num_forwards > 0)
> +       abort ();
> +      it->num_forwards++;
> +#endif
> +      break;
> +    }

Don't you want to increment num_forwards in the normal/expanded cases, too?

> +tokens_buff_append_token (cpp_reader *pfile,
> +                         _cpp_buff *buffer,
> +                         source_location *virt_locs,
> +                         const cpp_token *token,
> +                         source_location def_loc,
> +                         source_location parm_def_loc,
> +                         const struct line_map *map,
> +                         unsigned int *macro_token_index)

Why is macro_token_index a pointer?  Nothing seems to modify the referent.

> +/* Appends a token to the end of the token buffer BUFFER.  Note that
> +   this function doesn't enlarge BUFFER; it overwrite the last memory
> +   location of BUFFER that holds a token.

That doesn't sound like appending.

Jason