On Mon, Oct 1, 2012 at 11:01 AM, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.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">
<div class="gmail_extra"><div class="gmail_quote"><div class="im">On Mon, Oct 1, 2012 at 10:51 AM, Argyrios Kyrtzidis <span dir="ltr"><<a href="mailto:kyrtzidis@apple.com" target="_blank">kyrtzidis@apple.com</a>></span> wrote:<br>

<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Kim,<br>
<div><br>
On Sep 30, 2012, at 12:29 AM, Kim Gräsman <<a href="mailto:kim.grasman@gmail.com" target="_blank">kim.grasman@gmail.com</a>> wrote:<br>
<br>
> On Sat, Sep 29, 2012 at 11:50 PM, Kim Gräsman <<a href="mailto:kim.grasman@gmail.com" target="_blank">kim.grasman@gmail.com</a>> wrote:<br>
>><br>
>> I would expect either '<stdio.h>' or<br>
>> 'HEADER_NAME', but I think getting the expanded range ('<stdio.h>')<br>
>> would be the most natural.<br>
><br>
> Having looked into our callback implementation, we already handle<br>
> macro expansion, and I think passing down the macro name range<br>
> unexpanded gives more fidelity. So I change my mind -- I think getting<br>
> the range as-written would be better ('HEADER_NAME'). Of course,<br>
> assuming that FilenameRange.getBegin().isMacroID() is still true so<br>
> callback sinks can expand it.<br>
<br>
</div>Your suggestion is a bit contradictory, if we get the range 'HEADER_NAME', then FilenameRange.getBegin().isMacroID() will be false (since it's pointing at the beginning of the macro expansion).<br>
The most fidelity option would be to give a range for  '<stdio.h>' (MacroIDs for begin and end), from which a callback sink can get at the 'HEADER_NAME' file-level range.<br>
Is this your recommendation ?<br></blockquote><div><br></div></div><div>FWIW, I think it is, and it makes sense. ;] The source location structure is... confusing though.</div><div><br></div><div>Richard, thoughts?</div></div>
</div></blockquote><div><br></div><div><div>Yes, I think we should maintain SourceLocation fidelity wherever possible. Clients can convert to the expansion location themselves if they need to.</div><div><br></div><div>It looks like the CharEnd calculation for the string_literal and angle_string_literal cases is wrong:</div>
<div><br></div><div><div>    Filename = getSpelling(FilenameTok, FilenameBuffer);</div><div>    End = FilenameTok.getLocation();</div><div>    CharEnd = End.getLocWithOffset(Filename.size());</div></div><div><br></div><div>
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).</div>
</div></div>