<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><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><br></div><div>On May 8, 2013, at 8:19 AM, Manuel Klimek <<a href="mailto:klimek@google.com">klimek@google.com</a>> wrote:</div><br class="Apple-interchange-newline"><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>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><br></div><div>If there are more cases that we don't handle correctly, let me know.</div><div><br></div><div>-Argyrios</div></div></body></html>