From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30379 invoked by alias); 14 Sep 2011 21:41:11 -0000 Received: (qmail 30365 invoked by uid 22791); 14 Sep 2011 21:41:09 -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; Wed, 14 Sep 2011 21:40:50 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p8ELeOUG014416 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 14 Sep 2011 17:40:24 -0400 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id p8ELeM6k025045; Wed, 14 Sep 2011 17:40:23 -0400 Received: from [0.0.0.0] (ovpn-113-157.phx2.redhat.com [10.3.113.157]) by ns3.rdu.redhat.com (8.13.8/8.13.8) with ESMTP id p8ELeJsJ001107; Wed, 14 Sep 2011 17:40:19 -0400 Message-ID: <4E711F42.80802@redhat.com> Date: Wed, 14 Sep 2011 22:56: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> 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/msg00872.txt.bz2 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