On Fri, May 17, 2013 at 10:20 AM, Manuel Klimek <[email protected]> wrote:
> On Thu, May 16, 2013 at 11:41 PM, Argyrios Kyrtzidis <[email protected]>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 <[email protected]> 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!
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
