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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang/include/clang/Tooling/Syntax/Tokens.h:315
 private:
+  /// Maps from a start location to an end location of transformations performed
+  /// by the preprocessor. These include:
----------------
*spelled* locations!


================
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;
----------------
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


================
Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:262
+
+  /// Disabled instance will stop reporting anything to TokenCollector.
+  void disable() { Collector = nullptr; }
----------------
add a comment for *why* this is needed?


================
Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:269
+      return;
+    // Do not record recursive expansions.
+    if (!MacroNameTok.getLocation().isFileID() ||
----------------
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)


================
Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:522
   llvm::DenseMap<FileID, unsigned> NextSpelled;
+  PPExpansions Expansions;
   const SourceManager &SM;
----------------
maybe RecordedExpansions? to make the link with the recorder


================
Comment at: clang/unittests/Tooling/Syntax/TokensTest.cpp:431
   mappings:
-    ['#'_0, '<eof>'_18) => ['<eof>'_0, '<eof>'_0)
+    ['#'_0, 'EMPTY'_9) => ['<eof>'_0, '<eof>'_0)
+    ['EMPTY'_9, 'EMPTY_FUNC'_10) => ['<eof>'_0, '<eof>'_0)
----------------
the two #define statements are still merged into a single mapping.
Do we have a FIXME somewhere to cover this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62953