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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
