On 09/21/2011 09:34 AM, Dodji Seketeli wrote:
Jason Merrill<ja...@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
