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

[Jason Merrill <jason@redhat.com>]

On 09/18/2011 06:47 AM, Dodji Seketeli wrote:
> +    cpp_hashnode *macro;
> +
> +  }c;

Extra blank line, missing space.

>>> +/* 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.
> I have changed the name into tokens_buff_add_token and I updated the
> comments accordingly.

Hmm, it seems that what the callers are looking for is appending, so the 
name wasn't really the problem (though the new name is fine too).  And 
the new comment:

> +/* Adds a token at the end of the tokens contained in BUFFER.  Note
> +   that this function doesn't enlarge BUFFER when the number of tokens
> +   reaches BUFFER's size; it then overwrites the last memory location
> +   of BUFFER that holds a token.

clarifies the text that was confusing me so that it does sound more like 
appending except when the buffer is full.  But clobbering the last token 
when the buffer is full sounds like it's unlikely to be what the caller 
wants; should we abort instead?

> +  size_t expanded_capacity;     /* total size of expanded array.  */
> +  source_location *virt_locs;  /* Where virtual locations for
> +                                  unexpanded tokens are stored.  */
> +  unsigned virt_locs_capacity; /* Total size of virtual locations
> +                                  array.  */
> +  source_location *expanded_virt_locs; /* Where virtual locations for
> +                                         expanded tokens are
> +                                         stored.  */

Why do we need the capacity fields, when we didn't before?

> +         if (CPP_OPTION (pfile, track_macro_expansion))
> +           {
> +             unsigned int i, count = macro->count;
> +             const cpp_token *src = macro->exp.tokens;
> +             const struct line_map *map;
> +             source_location *virt_locs = NULL;
> +             _cpp_buff *macro_tokens =
> +               tokens_buff_new (pfile, count, &virt_locs);

This looks a lot like cloning tokens.  I thought you were storing 
virtual locations separately so you wouldn't have to do that?

Jason