From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1628 invoked by alias); 21 Sep 2011 18:33:26 -0000 Received: (qmail 1614 invoked by uid 22791); 21 Sep 2011 18:33:25 -0000 X-SWARE-Spam-Status: No, hits=-6.2 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, 21 Sep 2011 18:33:03 +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 p8LIWPBM021240 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 21 Sep 2011 14:32:25 -0400 Received: from localhost (ovpn-113-40.phx2.redhat.com [10.3.113.40]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id p8LIWMvw014367 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Wed, 21 Sep 2011 14:32:23 -0400 Received: by localhost (Postfix, from userid 500) id 3BB9C29C115; Wed, 21 Sep 2011 20:32:21 +0200 (CEST) From: Dodji Seketeli To: Jason Merrill 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 3/7] Emit macro expansion related diagnostics References: <1291979498-1604-1-git-send-email-dodji@redhat.com> <7ab852c58faea9efd81130c5a1ddc9e78b34bcc5.1310824121.git.dodji@redhat.com> <4E6E73F8.4030603@redhat.com> <4E74AA75.8090106@redhat.com> <4E778A26.1000707@redhat.com> <4E77ACA1.80205@redhat.com> <4E789C5B.20509@redhat.com> <4E793BF4.4010103@redhat.com> X-URL: http://www.redhat.com Date: Wed, 21 Sep 2011 19:09:00 -0000 In-Reply-To: <4E793BF4.4010103@redhat.com> (Jason Merrill's message of "Tue, 20 Sep 2011 21:20:52 -0400") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii 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/msg01273.txt.bz2 Jason Merrill writes: > My point was more that the name of the function is confusing; Sorry about that. > if what you get back is another virtual location, that's not what I > would consider a "def point". For tokens that are not function-like macro arguments, I think the linemap_macro_loc_to_def_point makes sense because what we get then is the source location in the actual definition of the macro. Actually I think that's where the confusion comes from. I first devised the name while thinking of the case of macros that are not function-like. And now it falls short for the general case. I should have caught that but for some reason I was just seeing through the name is if it was transparent. Sigh. I take your point; that name doesn't make sense in the general case. > The only time you get a source location in the actual definition of > the macro is when you ask for the "macro parm usage point". Yes that, and in the case above; in which case xI and yI are equal, by the way. > When we start out, we have a virtual location that represents << in > the expansion of OPERATE. Then we call > linemap_macro_map_loc_to_def_point to get a virtual location that > represents << in the expansion of SHIFTL. This is not part of the > definition of OPERATE, and shouldn't be described as such. Agreed. > It seems that this function steps out until we hit the spelling > location, and then we have a real source location and stop, which is > why the loop in maybe_unwind_expanded_macro_loc needs to use > linemap_macro_map_loc_to_exp_point as well. Right? Right. > It seems to me that we should encapsulate all of this in a function > that expresses this unwinding operation, say > "linemap_macro_map_loc_unwind_once". So the loop would look like > > + do > + { > + loc.where = where; > + loc.map = map; > + > + VEC_safe_push (loc_t, heap, loc_vec, &loc); > + > + /* WHERE is the location of a token inside the expansion of a > + macro. MAP is the map holding the locations of that macro > + expansion. Let's get the location of the token in the > + context that triggered the expansion of this macro. > + This is basically how we go "down" in the trace of macro > + expansions that led to WHERE. */ > + where = linemap_unwind_once (where, &map); > + } while (linemap_macro_expansion_map_p (map)); > OK. > I think that linemap_macro_loc_to_def_point when called with false > returns the unwound spelling location of the token (and thus is used > for LRK_SPELLING_LOCATION), Right. > or when called with true returns the (not-unwound) location in the > expanded macro (and so I think LRK_MACRO_PARM_REPLACEMENT_POINT > should be renamed to LRK_MACRO_EXPANDED_LOCATION or something > similar). FWIW, I'd like the LRK_MACRO_PARM_REPLACEMENT name (or its replacement. ha ha) to hint at the fact that it really has to do with a token that is an /argument/ for a function-like macro. So maybe LRK_MACRO_PARM_FOR_ARG_LOCATION? LRK_MACRO_EXPANDED_LOCATION really seems too vague to me. After all, pretty much everything is about *EXPAND*ing macros here. :-) > It seems to me that either we should split those functions apart in > to two functions with clearer names, or bundle them and > linemap_macro_loc_to_exp_point into linemap_resolve_location; if > linemap_location_in_system_header_p used linemap_resolve_location it > would be more readable anyway. OK. > I'm having trouble thinking of a less misleading name for > linemap_macro_map_loc_to_def_point, since the two locations serve > completely different purposes. Maybe something vague like > linemap_macro_map_loc_get_stored_loc. Or split it into two > functions like linemap_virtual_loc_unwind_once_toward_spelling and > linemap_virtual_loc_get_expanded_location or something like that. So would a function named linemap_macro_map_loc_to_def_point that returns the second location (yI) only, and a second function linemap_macro_map_loc_unwind_once be less confusing to you? If yes, then I'll send an updated patch for that in a short while. Thanks. -- Dodji