Page MenuHomePhabricator

[clang] Fix format specifiers fixits for nested macros
ClosedPublic

Authored by alexshap on Jun 15 2017, 7:56 PM.

Details

Summary

ExpansionLoc was previously calculated incorrectly in the case of
nested macros expansions. In this diff we build the stack of expansions where the last one
is the actual expansion (in the source code) which should be used for grouping together
the edits. The definition of MacroArgUse is adjusted accordingly: now it's essentially the stack of expansions plus
the location of argument use inside the top-most macro.
This diff fixes https://bugs.llvm.org/show_bug.cgi?id=33447 .

Test plan: make check-all

Diff Detail

Repository
rL LLVM

Event Timeline

alexshap created this revision.Jun 15 2017, 7:56 PM
mehdi_amini edited edge metadata.Jun 15 2017, 8:30 PM

Thanks for fixing! (I'm still not the best qualified to review this)

lib/Edit/EditedSource.cpp
78 ↗(On Diff #102773)

Nit: llvm::find_if allows to skip begin/end as llvm::find_if(I->second, [] ...

test/FixIt/fixit-format-darwin.m
71 ↗(On Diff #102773)

I think you mean pr instead of radar ;)

alexshap updated this revision to Diff 102781.Jun 15 2017, 9:05 PM

minor update

arphaman added inline comments.Jun 19 2017, 11:11 AM
include/clang/Edit/EditedSource.h
50 ↗(On Diff #102781)

I think a name like UseLoc is more appropriate.

lib/Edit/EditedSource.cpp
80 ↗(On Diff #102781)

Do you need to compare the entire expansion stack, or can you get away with just the comparison of the front of the stack?

alexshap updated this revision to Diff 103090.Jun 19 2017, 12:17 PM

Address code review comments

alexshap marked 3 inline comments as done.Jun 19 2017, 12:19 PM
alexshap added inline comments.
lib/Edit/EditedSource.cpp
80 ↗(On Diff #102781)

yeah, you are right, we can compare only the fronts because the entire stack can be recovered from that (via calling SourceMgr.getImmediateExpansionRange - like what's going on inside EditedSource::deconstructMacroArgLoc)

alexshap planned changes to this revision.Jun 19 2017, 12:30 PM
alexshap marked an inline comment as done.

probably we don't need to keep the entire expansion stack in MacroArgUse (i was doing that because it was easier to understand what's going on).
I will update this diff soon.

alexshap updated this revision to Diff 103093.Jun 19 2017, 12:46 PM

update, rerun the tests

arphaman accepted this revision.Jun 20 2017, 3:52 AM

LGTM.

This revision is now accepted and ready to land.Jun 20 2017, 3:52 AM
This revision was automatically updated to reflect the committed changes.