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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

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?



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


================
Comment at: clang/include/clang/Tooling/Syntax/Tokens.h:326
   Preprocessor &PP;
+  Callbacks *CB;
 };
----------------
Give the class and member more descriptive names?


================
Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:256
 
+class TokenCollector::Callbacks : public PPCallbacks {
+public:
----------------
what is this class for, what does it do?


================
Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:278
+private:
+  TokenCollector *C;
+  /// Used to detect recursive macro expansions.
----------------
members should have real names


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62953