[cfe-commits] r164874 - in /cfe/trunk: include/clang/Lex/PPCallbacks.h include/clang/Lex/PreprocessingRecord.h lib/Frontend/DependencyFile.cpp lib/Frontend/DependencyGraph.cpp lib/Lex/PPDirectives.cpp lib/Lex/PreprocessingRecord.cpp lib/Lex/Prepr

[Richard Smith]

On Mon, Oct 8, 2012 at 1:21 PM, Kim Gräsman <kim.grasman at gmail.com> wrote:

> Hi Richard, all,
>
> On Mon, Oct 1, 2012 at 8:31 PM, Richard Smith <richard at metafoo.co.uk>
> wrote:
> >
> > Yes, I think we should maintain SourceLocation fidelity wherever
> possible.
> > Clients can convert to the expansion location themselves if they need to.
> >
> > It looks like the CharEnd calculation for the string_literal and
> > angle_string_literal cases is wrong:
> >
> >     Filename = getSpelling(FilenameTok, FilenameBuffer);
> >     End = FilenameTok.getLocation();
> >     CharEnd = End.getLocWithOffset(Filename.size());
> >
> > You can't use getLocWithOffset to find the end of a token this way (it
> won't
> > work for trigraphs, at least). This should be using getLocForEndOfToken,
> > except that that apparently doesn't work inside a macro expansion. (That
> > looks like the same problem which is breaking Kim's angled-include-macro
> > case).
>
> Thanks for the hints!
>
> There seems to be two problems:
>
> 1) In the case of literal tokens, CharEnd was previously based on
> Filename.size(). This fails to take trigraphs into account. It looks
> like it works better with FilenameTok.getLength():
>
>    case tok::angle_string_literal:
>    case tok::string_literal:
>      Filename = getSpelling(FilenameTok, FilenameBuffer);
>      End = FilenameTok.getLocation();
> -    CharEnd = End.getLocWithOffset(Filename.size());
> +    CharEnd = End.getLocWithOffset(FilenameTok.getLength());
>      break;
>
>
> 2) In the case of the less token (triggered by macro expansions that
> result in angled includes), CharEnd was based on End, which had been
> advanced by ConcatenateIncludeName. End already points to the closing
> '>', so I changed it to:
>
>    case tok::less:
>      // This could be a <foo/bar.h> file coming from a macro expansion.
>  In this
>      // case, glue the tokens together into FilenameBuffer and interpret
> those.
>      FilenameBuffer.push_back('<');
>      if (ConcatenateIncludeName(FilenameBuffer, End))
>        return;   // Found <eod> but no ">"?  Diagnostic already emitted.
>      Filename = FilenameBuffer.str();
> -    CharEnd = getLocForEndOfToken(End);
> +
> +    CharEnd = End.getLocWithOffset(1);
>      break;
>
> I've tested these by running my callback impl over a simple .cc file,
> and I get the results I expect, i.e. the FilenameRange contains the
> macro "body" (this is the spelling location, right?);
>

Both of those changes look correct to me. [IIRC, the 'spelling location' is
what you get by flattening a (hierarchical) source location down to a
(non-hierarchical) location where the character literally appeared in a
source file, and that's not quite what you mean here, but close enough.]


> --
> #define MACRO_TRIGRAPH "trigraph??-empty.h"
> #define MACRO_ANGLED <vadefs.h>
> #define MACRO_QUOTED "empty.h"
> #define MACRO_STRINGIZED(x) #x
>
> #include MACRO_QUOTED
> #include MACRO_ANGLED
> #include MACRO_TRIGRAPH
> #include MACRO_STRINGIZED(empty.h)
>
> #include <vadefs.h>
> #include "empty.h"
> --
>
> (<vadefs.h> is an MSVC header that has no further includes, so as not
> to muddy the results)
>
> Is there test coverage for this stuff somewhere? I'm not familiar with
> Clang's testing tools yet, but I would make an effort to get this
> under test if I knew where to start...


I can't find any tests for this. I think the best way to proceed would be
to add a PPCallbacksTest.cpp to unittests/Lex.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121008/9921852c/attachment.html>