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

[Manuel Klimek]

On Fri, May 17, 2013 at 10:20 AM, Manuel Klimek <klimek at google.com> wrote:

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

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.

Thanks a lot!
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130517/b0afe9d3/attachment.html>