On Mon, Oct 8, 2012 at 1:21 PM, Kim Gräsman <[email protected]> wrote:
> Hi Richard, all, > > On Mon, Oct 1, 2012 at 8:31 PM, Richard Smith <[email protected]> > 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.
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
