From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12834 invoked by alias); 22 Sep 2011 16:50:59 -0000 Received: (qmail 12824 invoked by uid 22791); 22 Sep 2011 16:50:58 -0000 X-SWARE-Spam-Status: No, hits=-6.7 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,SPF_HELO_PASS X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 22 Sep 2011 16:50:37 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p8MGoBMI002064 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 22 Sep 2011 12:50:11 -0400 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id p8MGo9Rc011948; Thu, 22 Sep 2011 12:50:10 -0400 Received: from [0.0.0.0] (ovpn-113-36.phx2.redhat.com [10.3.113.36]) by ns3.rdu.redhat.com (8.13.8/8.13.8) with ESMTP id p8MGo6xs024424; Thu, 22 Sep 2011 12:50:06 -0400 Message-ID: <4E7B673D.9050306@redhat.com> Date: Thu, 22 Sep 2011 17:10:00 -0000 From: Jason Merrill User-Agent: Mozilla/5.0 (X11; Linux i686; rv:6.0.2) Gecko/20110906 Thunderbird/6.0.2 MIME-Version: 1.0 To: Dodji Seketeli CC: gcc-patches@gcc.gnu.org, tromey@redhat.com, gdr@integrable-solutions.net, joseph@codesourcery.com, burnus@net-b.de, charlet@act-europe.fr, bonzini@gnu.org Subject: Re: [PATCH 2/7] Generate virtual locations for tokens References: <1291979498-1604-1-git-send-email-dodji@redhat.com> <4E6E65B2.2060909@redhat.com> <4E711F42.80802@redhat.com> <4E77ACFE.2080805@redhat.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org X-SW-Source: 2011-09/txt/msg01345.txt.bz2 On 09/21/2011 09:34 AM, Dodji Seketeli wrote: > Jason Merrill 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