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

[Jason Merrill <jason@redhat.com>]

On 09/21/2011 09:34 AM, Dodji Seketeli wrote:
> Jason Merrill<jason@redhat.com>  writes:

>> clobbering the last token when the buffer is full sounds like it's
>> unlikely to be what the caller wants; should we abort instead?
>
> abort () added in that case.

Please update the comment as well.

> +/* An iterator over tokens coming from a function line macro

"function-like"

> +  /* The function-like macro the tokens come from.  */
> +  const macro_arg *arg;

This field doesn't seem to be used anywhere.

> +  /* The cpp_reader the macro comes from.  */
> +  cpp_reader *pfile;

This seems to only be used to decide whether or not to increment 
location_ptr.  Rather than base that decision on going all the way back 
to the CPP_OPTION, let's just pass a flag to macro_arg_token_iter_init 
and use that to decide whether or not to set location_ptr.

> +/* Return the location of the token pointed to by the iterator.*/
> +static source_location
> +macro_arg_token_iter_get_location (const macro_arg_token_iter *it)
> +{
> +#ifdef ENABLE_CHECKING
> +  if (it->kind == MACRO_ARG_TOKEN_STRINGIFIED
> +      && it->num_forwards > 0)
> +    abort ();
> +#endif
> +  return *it->location_ptr;
> +}

And then here if location_ptr isn't set we should get the location from 
the token.

> +  if (virt_location)
> +    {
> +      if (track_macro_exp_p)
> +       {
> +         if (kind == MACRO_ARG_TOKEN_NORMAL)
> +           *virt_location = &arg->virt_locs[index];
> +         else if (kind == MACRO_ARG_TOKEN_EXPANDED)
> +           *virt_location = &arg->expanded_virt_locs[index];
> +         else if (kind == MACRO_ARG_TOKEN_STRINGIFIED)
> +           *virt_location =
> +             (source_location *) &tokens_ptr[index]->src_loc;
> +       }
> +      else

Similarly, here virt_location should only be set when we're tracking 
macro expansions, so the second test becomes redundant.

> +tokens_buff_new (cpp_reader *pfile, size_t len,
> +                source_location **virt_locs)
> +{
> +  bool track_macro_exp_p = CPP_OPTION (pfile, track_macro_expansion);
> +  size_t tokens_size = len * sizeof (cpp_token *);
> +  size_t locs_size = len * sizeof (source_location);
> +
> +  if (track_macro_exp_p && virt_locs != NULL)
> +    *virt_locs = XNEWVEC (source_location, locs_size);

And here.

> +           *num_args = num_args_alloced;;

Extra ;

> +      num_args_alloced++;
>
>        argc++;

How does num_args_alloced differ from argc?

> +  result =
> +    tokens_buff_put_token_to (pfile, (const cpp_token **) BUFF_FRONT (buffer),
> +                             &virt_locs[token_index],
> +                             token, def_loc, parm_def_loc,
> +                             map, macro_token_index);

Here if virt_locs is null we should pass down null as well.

> +  if (track_macro_exp_p)
> +    {
> +      if (map)
> +       macro_loc = linemap_add_macro_token (map, macro_token_index,
> +                                            def_loc, parm_def_loc);
> +      *virt_loc_dest = macro_loc;
> +    }

So that here again we can just check that virt_loc_dest is set rather 
than the CPP_OPTION.

>    pfile->context = context->prev;
> +  /* decrease peak memory consumption by feeing the context.  */
> +  pfile->context->next = NULL;
> +  free (context);

Setting pfile->context->next to NULL seems wrong; either it's already 
NULL or we're making something unreachable.

> +   LOC is an out parameter; *LOC is set to the location "as expected
> +   by the user".  */

This is puzzling without the explanation before 
cpp_get_token_with_location; just refer to that comment here.

In cpp_get_token_1 the distinction between code paths that set virt_loc 
and those that set *location directly seems unfortunate; I would think 
it would be cleaner to do

> +  if (location)
       {
	if (virt_loc == 0) virt_loc = result->src_loc;
> +    *location = virt_loc;

and drop the direct settings of *location/gotos earlier in the function.

Jason