On Sep 17, 2012, at 2:38 PM, James Dennett <[email protected]> wrote:
> On Mon, Sep 17, 2012 at 2:22 PM, Argyrios Kyrtzidis <[email protected]> > wrote: >> On Sep 17, 2012, at 12:44 PM, James Dennett <[email protected]> wrote: >> >>> On Mon, Sep 10, 2012 at 7:17 PM, Argyrios Kyrtzidis <[email protected]> >>> wrote: >>>> Author: akirtzidis >>>> Date: Mon Sep 10 21:17:21 2012 >>>> New Revision: 163588 >>>> >>>> URL: http://llvm.org/viewvc/llvm-project?rev=163588&view=rev >>>> Log: >>>> [libclang] Fix getting a cursor inside an angled #include directive. >>>> >>>> Fixed by pointing the end location of the preprocessed entity for the >>>> #include >>>> at the closing '>', instead of the start of '<'. >>>> >>>> rdar://11113134 >>>> >>>> Modified: >>>> cfe/trunk/lib/Lex/PPDirectives.cpp >>>> cfe/trunk/test/Index/c-index-getCursor-pp.c >>>> >>>> Modified: cfe/trunk/lib/Lex/PPDirectives.cpp >>>> URL: >>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Lex/PPDirectives.cpp?rev=163588&r1=163587&r2=163588&view=diff >>>> ============================================================================== >>>> --- cfe/trunk/lib/Lex/PPDirectives.cpp (original) >>>> +++ cfe/trunk/lib/Lex/PPDirectives.cpp Mon Sep 10 21:17:21 2012 >>>> @@ -1296,6 +1296,9 @@ >>>> case tok::string_literal: >>>> Filename = getSpelling(FilenameTok, FilenameBuffer); >>>> End = FilenameTok.getLocation(); >>>> + // For an angled include, point the end location at the closing '>'. >>>> + if (FilenameTok.is(tok::angle_string_literal)) >>>> + End = End.getLocWithOffset(Filename.size()-1); >>>> CharEnd = End.getLocWithOffset(Filename.size()); >>>> break; >>>> >>>> >>>> Modified: cfe/trunk/test/Index/c-index-getCursor-pp.c >>>> URL: >>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Index/c-index-getCursor-pp.c?rev=163588&r1=163587&r2=163588&view=diff >>>> ============================================================================== >>>> --- cfe/trunk/test/Index/c-index-getCursor-pp.c (original) >>>> +++ cfe/trunk/test/Index/c-index-getCursor-pp.c Mon Sep 10 21:17:21 2012 >>>> @@ -15,6 +15,8 @@ >>>> >>>> const char *fname = __FILE__; >>>> >>>> +#include <a.h> >>>> + >>>> // RUN: c-index-test -cursor-at=%s:1:11 -I%S/Inputs %s | FileCheck >>>> -check-prefix=CHECK-1 %s >>>> // CHECK-1: macro definition=OBSCURE >>>> // RUN: c-index-test -cursor-at=%s:2:14 -I%S/Inputs %s | FileCheck >>>> -check-prefix=CHECK-2 %s >>>> @@ -31,6 +33,8 @@ >>>> // CHECK-7: macro expansion=B:12:9 >>>> // RUN: c-index-test -cursor-at=%s:16:25 -I%S/Inputs %s | FileCheck >>>> -check-prefix=CHECK-8 %s >>>> // CHECK-8: macro expansion=__FILE__ >>>> +// RUN: c-index-test -cursor-at=%s:18:12 -I%S/Inputs %s | FileCheck >>>> -check-prefix=CHECK-9 %s >>>> +// CHECK-9: inclusion directive=a.h >>>> >>>> // Same tests, but with "editing" optimizations >>>> // RUN: env CINDEXTEST_EDITING=1 c-index-test -cursor-at=%s:1:11 >>>> -I%S/Inputs %s | FileCheck -check-prefix=CHECK-1 %s >>> >>> This change broke clients that implement >>> PPCallbacks::InclusionDirective, which now receive the location of the >>> closing '>' rather than the location of the token that they're >>> expecting based on the spec: >>> /// \param EndLoc The location of the last token within the >>> inclusion >>> /// directive. >>> The last (preprocessing) token is the whole of <filename>, of type >>> tok::angle_string_literal. >>> >>> Could we consider reverting this change and finding an alternative way >>> to fix libclang without breaking/changing the lower-level interface? >> >> Hi James, >> >> This is not a high-level libclang issue. > > Apologies, I was going by the change description "[libclang] Fix > getting a cursor inside an angled #include directive.", and that the > only tests I saw included were for libclang. (That does suggests that > we're missing tests for this in the appropriate place.) You're right, a preprocessing record unit test is also needed. > >> The preprocessing record needs to know the '>' location and depends on the >> PPCallbacks::InclusionDirective callback. Previously this callback did not >> provide a way to reliably get at that location. > > I'm not sure about that -- it provided a way to get there, but it was > necessary to skip over the filename. If the include filename was formed via a macro expansion then you had an EndLoc pointing at '>'. So this was the situation: No macro expansions involved: EndLoc points at '<' With macro expansion: EndLoc points at '>'. The receiver had no easy way to know which case it was. With the change it always pointed at '>'. > With your change a similar trick > is required for code wanting to get to the filename, but only for > #include <filename>, not for #include "filename". (Why are the two > cases different? Currently the callback returns the location of the > opening quote for the #include "name" case, and the closing one for > #include <name>. The asymmetry seems strange.) There's an asymmetry due to how lexing would behave if you pointed at the beginning of the filename, for example: #include "filename" If you point at the opening quote and start lexing you will get the tok::string_literal token, the same that the preprocessor saw. Then you can get at the character range of the whole filename input by checking the token size. #include <filename> if you point at the opening '<' and start lexing you will get a tok::less token, you need to get into the special "parsing include directive" mode to receive a tok::angle_string_literal token. So just pointing at '<' will not allow you to easily lex and get the character range of the filename input. > >> I believe we still need to modify the bahavior of the InclusionDirective >> callback and focus the discussion there. >> >> How about reverting the EndLoc adjustment but have the InclusionDirective >> also pass the FilenameTok token so that receivers of the callback can have >> more information for the inclusion directive (then the preprocessing record >> can do the adjustment itself). > > That sounds good to me -- though I'm not sure if we need to pass > EndLoc if we pass the FilenameTok (possibly for cases where the > filename comes from macro expansion? I'm not all that familiar with > this code). Yes, the EndLoc is necessary because if there are macro expansions involved then the FilenameTok token will be a tok::less for '<'. > I believe that passing the FilenameTo to > InclusionDirective() would make life easier for consumers. Leaving > EndLoc there is the easy option, but I'm not sure if that's just > leaving detritus behind. > > -- James _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
