Page MenuHomePhabricator

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

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



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?


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.


Give the class and member more descriptive names?


what is this class for, what does it do?


members should have real names

ilya-biryukov marked 7 inline comments as done.Jun 19 2019, 3:40 AM
ilya-biryukov added inline comments.

Renamed to PPExpansions.


Renamed to Expansions


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.

*spelled* locations!


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


add a comment for *why* this is needed?


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)


maybe RecordedExpansions? to make the link with the recorder


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.

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


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


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