<div dir="ltr">On Fri, May 17, 2013 at 10:20 AM, Manuel Klimek <span dir="ltr"><<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div class="h5">On Thu, May 16, 2013 at 11:41 PM, Argyrios Kyrtzidis <span dir="ltr"><<a href="mailto:akyrtzi@gmail.com" target="_blank">akyrtzi@gmail.com</a>></span> wrote:<br>
</div></div><div class="gmail_extra"><div class="gmail_quote"><div><div class="h5">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div>Hi Manuel,</div><div><br></div><div>I committed r182055 with a modified version of your patch. It also fixes a couple of cases that were not handled correctly:</div>
<div><br></div><div>-if the range overlapped the arguments of 2 function macros it would give a valid range instead of giving up, e.g:</div><div><br></div><div><font face="Monaco">#define M(x) x<br>M(c M(i)) M(M(i) c)</font></div>
<div><br></div><div>a range of the two 'i''s would give this range</div><div><br></div><div><div><font face="Monaco">M(c M(i)) M(M(i) c)</font></div></div><div><font face="Monaco"> ^----------^</font></div>
<div><br></div><div>which is not good.</div><div><br></div><div>-It would give up if the range resided in one macro argument but did not have expansions in both ends, e.g:</div><div><br></div><div><font face="Monaco">#define M(x) x<br>
M(M(i) c c)</font></div><div><br></div><div>it would give up if you asked for the range of ['i', 'c'], instead of returning this</div><div><div><font face="Monaco">M(M(i) c c)</font></div></div><div><font face="Monaco"> ^----^</font></div>
<div><br></div><div>More commenting below..</div><div><div><div><br></div><div>On May 8, 2013, at 8:19 AM, Manuel Klimek <<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>> wrote:</div>
<br><blockquote type="cite">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></blockquote><div><br>
</div>
</div></div><div>I think you were on the right track but we can use the previous/next FileID to check instead using locations like that.</div><div>And since this is very dependent in internal mechanisms of the SourceManager I abstracted such logic in a couple of helper functions in SourceManager.</div>
</div></div></blockquote><div><br></div></div></div><div>Very cool. I tried to implement it that way, too, but thought it was impossible to go through the file ids :)</div><div class="im"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><div><div>If there are more cases that we don't handle correctly, let me know.</div></div></div></blockquote><div><br></div></div><div>I'm wondering how you got around looping over the spelling chain, so I don't understand yet how you'll always find the outermost expansion that spans the full range. I'll check with some more test cases...</div>
</div></div></div></blockquote><div><br></div><div style>Wow, /me completely missed the recursion in the end ... :) Now that I understand it, looks great, and I wasn't able to come up with tests that still fail so far.</div>
<div style><br></div><div style>Thanks a lot!</div><div style><br></div></div></div></div>