Page MenuHomePhabricator

[Syntax] Do not glue multiple empty PP expansions to a single mapping
ClosedPublic

Authored by ilya-biryukov on Jun 6 2019, 7:22 AM.

Details

Summary

This change makes sure we have a single mapping for each macro expansion,
even if the result of expansion was empty.

To achieve that, we take information from PPCallbacks::MacroExpands into
account. Previously we relied only on source locations of expanded tokens.

Event Timeline

ilya-biryukov created this revision.Jun 6 2019, 7:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 6 2019, 7:22 AM

Can you explain why this is important?
(in the code)

  • Added comments.

Can you explain why this is important?
(in the code)

I've added a few comments into the code that builds token buffers, but I couldn't figure out a good place to mention this in the API docs.
It would be a natural expectation from the API from my POV, the fact that we glued empty expansions together is an implementation artifact rather than a sensible contract...

Sorry, I'm having trouble understanding this patch. Can you try to find some clearer names for the new concepts, or describe how they differ?

clang/include/clang/Tooling/Syntax/Tokens.h
319

There are now 4 things called mappings, and I can't understand how they relate to each other. I think this needs new names and/or concepts.

341

Give the class and member more descriptive names?

clang/lib/Tooling/Syntax/Tokens.cpp
256

what is this class for, what does it do?

278

members should have real names

ilya-biryukov marked 7 inline comments as done.Jun 19 2019, 3:40 AM
ilya-biryukov added inline comments.
clang/include/clang/Tooling/Syntax/Tokens.h
319

Renamed to PPExpansions.

341

Renamed to Expansions

clang/lib/Tooling/Syntax/Tokens.cpp
256

Added a comment.

ilya-biryukov marked 3 inline comments as done.
  • Address comments
ilya-biryukov added a comment.EditedJun 19 2019, 6:26 AM

To clarify, I don't think there are new concepts in this patch.
Previously, we only took information from source locations into account when building token buffers. That works fine in most cases, but not enough to find the boundaries of empty macro expansions.
In order to find those, we now also watch the callbacks from the preprocessor to get all macro expansions (including the empty ones) and reconstruct the mappings of token buffers from there.

sammccall accepted this revision.Jun 24 2019, 4:45 AM
sammccall added inline comments.
clang/include/clang/Tooling/Syntax/Tokens.h
316

*spelled* locations!

320

do I understand right that this is logically a stack, but it's hard to know when to pop or just less hassle to do this way?
if so, maybe worth mentioning

clang/lib/Tooling/Syntax/Tokens.cpp
262

add a comment for *why* this is needed?

269

This doesn't seem like a particularly standard use of the word "recursive", and the code isn't totally obvious either.

Could this be "only record top-level expansions, not those where:

  • the macro use is inside a macro body
  • the macro appears in an argument to another macro

Because the top level macros are treated as opaque atoms. (We probably need a FIXME for tracking arg locations somewhere)

526

maybe RecordedExpansions? to make the link with the recorder

clang/unittests/Tooling/Syntax/TokensTest.cpp
440–442

the two #define statements are still merged into a single mapping.
Do we have a FIXME somewhere to cover this?

This revision is now accepted and ready to land.Jun 24 2019, 4:45 AM
ilya-biryukov marked 10 inline comments as done.
  • Address comments, document code.
  • s/Expansion/CollectedExpansions.
  • Added FIXMEs for macro arguments.
clang/include/clang/Tooling/Syntax/Tokens.h
320

That's exactly the case, but preprocessor only exposes the point at which we push macros to the stack (PPCallbacks::MacroExpands, etc) and not points when we pop from the stack. This map is an attempt to recover the pop positions (e.g. to detect intermediate expansions in the macro arguments).

Added a comment

clang/lib/Tooling/Syntax/Tokens.cpp
269

Added a comment and a FIXME at the declaration site of TokenBuffer

526

SG, I've renamed to CollectedExpansions. (Assuming 'recorder' stands for 'collector', happy to update if I misinterpreted your comment)

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 24 2019, 2:41 PM
chrish_ericsson_atx added inline comments.
clang/lib/Tooling/Syntax/Tokens.cpp
451

What is intended by this line?

ilya-biryukov marked 2 inline comments as done.Fri, Nov 15, 10:48 PM
ilya-biryukov added inline comments.
clang/lib/Tooling/Syntax/Tokens.cpp
451

Suppressing compiler warning for unused local variable.