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.) > 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. 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.) > 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). 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
