[cfe-commits] r163588 - in /cfe/trunk: lib/Lex/PPDirectives.cpp test/Index/c-index-getCursor-pp.c

[James Dennett]

On Mon, Sep 17, 2012 at 2:22 PM, Argyrios Kyrtzidis <kyrtzidis at apple.com> wrote:
> On Sep 17, 2012, at 12:44 PM, James Dennett <jdennett at googlers.com> wrote:
>
>> On Mon, Sep 10, 2012 at 7:17 PM, Argyrios Kyrtzidis <akyrtzi at gmail.com> 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