On Mon, Oct 8, 2012 at 1:21 PM, Kim Gräsman <span dir="ltr"><<a href="mailto:kim.grasman@gmail.com" target="_blank">kim.grasman@gmail.com</a>></span> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi Richard, all,<br>
<div class="im"><br>
On Mon, Oct 1, 2012 at 8:31 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br>
><br>
> Yes, I think we should maintain SourceLocation fidelity wherever possible.<br>
> Clients can convert to the expansion location themselves if they need to.<br>
><br>
> It looks like the CharEnd calculation for the string_literal and<br>
> angle_string_literal cases is wrong:<br>
><br>
> Filename = getSpelling(FilenameTok, FilenameBuffer);<br>
> End = FilenameTok.getLocation();<br>
> CharEnd = End.getLocWithOffset(Filename.size());<br>
><br>
> You can't use getLocWithOffset to find the end of a token this way (it won't<br>
> work for trigraphs, at least). This should be using getLocForEndOfToken,<br>
> except that that apparently doesn't work inside a macro expansion. (That<br>
> looks like the same problem which is breaking Kim's angled-include-macro<br>
> case).<br>
<br>
</div>Thanks for the hints!<br>
<br>
There seems to be two problems:<br>
<br>
1) In the case of literal tokens, CharEnd was previously based on<br>
Filename.size(). This fails to take trigraphs into account. It looks<br>
like it works better with FilenameTok.getLength():<br>
<br>
case tok::angle_string_literal:<br>
case tok::string_literal:<br>
<div class="im"> Filename = getSpelling(FilenameTok, FilenameBuffer);<br>
End = FilenameTok.getLocation();<br>
</div>- CharEnd = End.getLocWithOffset(Filename.size());<br>
+ CharEnd = End.getLocWithOffset(FilenameTok.getLength());<br>
break;<br>
<br>
<br>
2) In the case of the less token (triggered by macro expansions that<br>
result in angled includes), CharEnd was based on End, which had been<br>
advanced by ConcatenateIncludeName. End already points to the closing<br>
'>', so I changed it to:<br>
<br>
case tok::less:<br>
// This could be a <foo/bar.h> file coming from a macro expansion. In this<br>
// case, glue the tokens together into FilenameBuffer and interpret those.<br>
FilenameBuffer.push_back('<');<br>
if (ConcatenateIncludeName(FilenameBuffer, End))<br>
return; // Found <eod> but no ">"? Diagnostic already emitted.<br>
Filename = FilenameBuffer.str();<br>
- CharEnd = getLocForEndOfToken(End);<br>
+<br>
+ CharEnd = End.getLocWithOffset(1);<br>
break;<br>
<br>
I've tested these by running my callback impl over a simple .cc file,<br>
and I get the results I expect, i.e. the FilenameRange contains the<br>
macro "body" (this is the spelling location, right?);<br></blockquote><div><br></div><div>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.]</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
--<br>
#define MACRO_TRIGRAPH "trigraph??-empty.h"<br>
#define MACRO_ANGLED <vadefs.h><br>
#define MACRO_QUOTED "empty.h"<br>
#define MACRO_STRINGIZED(x) #x<br>
<br>
#include MACRO_QUOTED<br>
#include MACRO_ANGLED<br>
#include MACRO_TRIGRAPH<br>
#include MACRO_STRINGIZED(empty.h)<br>
<br>
#include <vadefs.h><br>
#include "empty.h"<br>
--<br>
<br>
(<vadefs.h> is an MSVC header that has no further includes, so as not<br>
to muddy the results)<br>
<br>
Is there test coverage for this stuff somewhere? I'm not familiar with<br>
Clang's testing tools yet, but I would make an effort to get this<br>
under test if I knew where to start...</blockquote><div><br></div><div>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. </div></div>