[PATCH] Fix makeFileCharRange for macro arguments in the presence of nested macro calls.

[Manuel Klimek]

On Thu, May 16, 2013 at 11:41 PM, Argyrios Kyrtzidis <akyrtzi at gmail.com>wrote:

> Hi Manuel,
>
> I committed r182055 with a modified version of your patch. It also fixes a
> couple of cases that were not handled correctly:
>
> -if the range overlapped the arguments of 2 function macros it would give
> a valid range instead of giving up, e.g:
>
> #define M(x) x
> M(c M(i)) M(M(i) c)
>
> a range of the two 'i''s would give this range
>
> M(c M(i)) M(M(i) c)
>     ^----------^
>
> which is not good.
>
> -It would give up if the range resided in one macro argument but did not
> have expansions in both ends, e.g:
>
> #define M(x) x
> M(M(i) c c)
>
> it would give up if you asked for the range of ['i', 'c'], instead of
> returning this
> M(M(i) c c)
>   ^----^
>
> More commenting below..
>
> On May 8, 2013, at 8:19 AM, Manuel Klimek <klimek at google.com> wrote:
>
> Hi doug.gregor,
>
> Added tests that show the problems:
> 1. isAt(Start|End)OfMacroExpansion did not work correctly for nested macro
>   calls; consider
>   #define M(x) x
>   #define N(x) x
>   N(f(M(i)))
>   Here getSourceText would return "M(f(M(i)))" for the token location of
> 'i',
>   as makeFileCharRange would consider 'i' to be at the start of the macro.
>   The problem is that getDecomposedLoc in this case returns the offset from
>   the inner call to 'M', and I have not found a way to get the offset from
>   'N' easily, as this information seems to be incrementally overwritten
> during
>   preprocessing.
>   The solution is to instead take the location before / after the examined
>   location and check whether its expansion location is the same as the
>   expansion location of the examined location. If this is the case, the
>   location before / after is not the first / last token of the macro
> expansion.
>
> 2. makeFileCharRange only considered the outermost layer of macro
> expansion,
>   thus not returning the widest possible range if the outermost expansion
>   was not fully describing the range.
>   After fixing (1), this would lead to getSourceText returning an empty
>   string instead of "M(i)".
>
> Note that I'm still not sure I'm entirely happy with this implementation.
> So far I have not found a good way to figure out whether the location
> is good to call getLocWithOffset(-1) or getLocWithOffset(tokLen + 1) on.
> Especially for the latter, the check for SM.isLocalSourceLocation(...)
> seems
> to work, but doesn't seem like the right solution. Any hints for a better
> solution would be highly welcome.
>
>
> I think you were on the right track but we can use the previous/next
> FileID to check instead using locations like that.
> And since this is very dependent in internal mechanisms of the
> SourceManager I abstracted such logic in a couple of helper functions in
> SourceManager.
>

Very cool. I tried to implement it that way, too, but thought it was
impossible to go through the file ids :)


> If there are more cases that we don't handle correctly, let me know.
>

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...

Cheers,
/Manuel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130517/7fd590ca/attachment.html>