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

[Argyrios Kyrtzidis]

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.

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

-Argyrios
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130516/de82bf87/attachment.html>