<html><head><meta http-equiv="Content-Type" content="text/html charset=iso-8859-1"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div>Hi Manuel,</div><div><br></div><div>The tests are great, give me some time and I'll investigate and comment on your patch soon(ish).</div><br><div><div>On May 13, 2013, at 12:23 AM, Manuel Klimek <<a href="mailto:klimek@google.com">klimek@google.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div dir="ltr">+Argyrios</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, May 8, 2013 at 5:19 PM, Manuel Klimek <span dir="ltr"><<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex; position: static; z-index: auto; ">Hi doug.gregor,<br>
<br>
Added tests that show the problems:<br>
1. isAt(Start|End)OfMacroExpansion did not work correctly for nested macro<br>
calls; consider<br>
#define M(x) x<br>
#define N(x) x<br>
N(f(M(i)))<br>
Here getSourceText would return "M(f(M(i)))" for the token location of 'i',<br>
as makeFileCharRange would consider 'i' to be at the start of the macro.<br>
The problem is that getDecomposedLoc in this case returns the offset from<br>
the inner call to 'M', and I have not found a way to get the offset from<br>
'N' easily, as this information seems to be incrementally overwritten during<br>
preprocessing.<br>
The solution is to instead take the location before / after the examined<br>
location and check whether its expansion location is the same as the<br>
expansion location of the examined location. If this is the case, the<br>
location before / after is not the first / last token of the macro expansion.<br>
<br>
2. makeFileCharRange only considered the outermost layer of macro expansion,<br>
thus not returning the widest possible range if the outermost expansion<br>
was not fully describing the range.<br>
After fixing (1), this would lead to getSourceText returning an empty<br>
string instead of "M(i)".<br>
<br>
Note that I'm still not sure I'm entirely happy with this implementation.<br>
So far I have not found a good way to figure out whether the location<br>
is good to call getLocWithOffset(-1) or getLocWithOffset(tokLen + 1) on.<br>
Especially for the latter, the check for SM.isLocalSourceLocation(...) seems<br>
to work, but doesn't seem like the right solution. Any hints for a better<br>
solution would be highly welcome.<br>
<br>
<a href="http://llvm-reviews.chandlerc.com/D767" target="_blank">http://llvm-reviews.chandlerc.com/D767</a><br>
<br>
Files:<br>
lib/Lex/Lexer.cpp<br>
unittests/Lex/LexerTest.cpp<br>
</blockquote></div><br></div>
</blockquote></div><br></body></html>