This is an archive of the discontinued LLVM Phabricator instance.

Fix isInSystemMacro to handle pasted token
ClosedPublic

Authored by serge-sans-paille on Dec 17 2018, 12:00 PM.

Details

Summary

Token pasted by the preprocessor (through ##) have a Spelling pointing to scratch buffer. As a result they are not recognized at system macro, even though the pasting happened in a system macro. Fix that by looking into the parent macro if the original lookup is not enough.

This effectively fixes https://bugs.llvm.org/show_bug.cgi?id=35268, but I'm not quite sure about the kind of test case I should add with this patch?

Diff Detail

Repository
rL LLVM

Event Timeline

serge-sans-paille retitled this revision from Fix isInSystemMacro to handle pasted toekn to Fix isInSystemMacro to handle pasted token.Dec 17 2018, 12:15 PM

Test case added

rsmith added inline comments.Jan 28 2019, 11:04 AM
include/clang/Basic/SourceManager.h
1458–1460 ↗(On Diff #181498)

I think this also matches things like:

// system header
#define BAR FOO

// user code
#define FOO x
BAR

... where the location of x token produced by the BAR macro will now be considered to be in a system macro. Can you make this more targeted? (Maybe we could change the SLocEntry for the token-paste scratch buffer to be considered a system header if the ## token is in a system macro -- that'll need the preprocessor to store two ScratchBuffers, one for system headers and one for user code, but that seems acceptable.)

Patch updated to take into account @rsmith remarks. Test case added as an illustration.

serge-sans-paille marked 2 inline comments as done.Jan 30 2019, 4:41 AM
serge-sans-paille added inline comments.
include/clang/Basic/SourceManager.h
1458–1460 ↗(On Diff #181498)

Thanks for the advice! This approach should be more efficient and avoid the situation you point out - I've added a test case to illustrate that.

rsmith accepted this revision.Jan 31 2019, 1:17 PM
rsmith added inline comments.
include/clang/Basic/SourceManager.h
1466 ↗(On Diff #184275)

Please add a space after if.

This revision is now accepted and ready to land.Jan 31 2019, 1:17 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJan 31 2019, 10:11 PM
serge-sans-paille marked an inline comment as done.Jan 31 2019, 10:36 PM

It's in, thanks @rsmith for the review.