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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clang/include/clang/Tooling/Syntax/Tokens.h:319
+  ///   2. macro name and arguments for macro expansions.
+  using PPExpansions = llvm::DenseMap</*SourceLocation*/ int, SourceLocation>;
   class Builder;
----------------
sammccall wrote:
> 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
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


================
Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:269
+      return;
+    // Do not record recursive expansions.
+    if (!MacroNameTok.getLocation().isFileID() ||
----------------
sammccall wrote:
> 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)
Added a comment and a FIXME at the declaration site of `TokenBuffer`


================
Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:522
   llvm::DenseMap<FileID, unsigned> NextSpelled;
+  PPExpansions Expansions;
   const SourceManager &SM;
----------------
sammccall wrote:
> maybe RecordedExpansions? to make the link with the recorder
SG, I've renamed to `CollectedExpansions`. (Assuming 'recorder' stands for 'collector', happy to update if I misinterpreted your comment)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62953/new/

https://reviews.llvm.org/D62953