Closed bruntib closed 5 years ago
Can one of the admins verify this patch?
Me too, I'd require the author to check this fix. For example I don't know if there is a better identification of tokens besides their names. @Szelethus
Thank you so much for taking care of this!
Instead of gathering strings, we'd be better off with storing const IdentifierInfo
s. You can see taht the main loop that iterates over the macro expansion's tokens has an if (Tok.is(tok::identifieer))
statement, meaning that the code it guards ensures that
Tok
is an tok:identifier
,IdentifierInfo
, and for each occurance of that given token the same identifierinfo will be returned,
so gathering these in a set would be, I imagine, far more efficient than working with strings.Also, please include a test if it's feasible. Does check-clang-analysis
run cleanly?
Can one of the admins verify this patch?
run tests
@bruntib
Can one of the admins verify this patch?
This is just a message from Jenkins, Dani is still on the ski slopes I think :D
Well, I'm not listed as a reviewer, but this LGTM. Thanks!
@Szelethus thanks for the review comments. At the time of creating this pull request, you were not a collaborator to this repository, and I couldn't add you. I meant you as the author of the macro feature.
@martong I run the tests with make check-clang
. I haven't created test for this specific recursive case. The goal for now is to include this fix in our internal test frameworks which is scheduled at 21:00.
@bruntib @Szelethus just added you to the collaborators
define f(y) x define x f(x)
This example results a preprocessor error since "x" in the first line was not defined earlier. However, the macro expression printer goes to an infinite recursion on this example.