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... Cheers, /Manuel
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
