sammccall created this revision. sammccall added reviewers: kadircet, vabridgers. Herald added a project: clang. Herald added a subscriber: cfe-commits. sammccall added a parent revision: D77614: [Syntax] Simplify TokenCollector::Builder, use captured expansion bounds. NFC.
Our previous definition of "top-level" was too informal, and didn't allow for overlapping macros that each directly produce expanded tokens. See D77507 <https://reviews.llvm.org/D77507> for previous discussion. Fixes http://bugs.llvm.org/show_bug.cgi?id=45428 Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D77615 Files: clang/lib/Tooling/Syntax/Tokens.cpp clang/unittests/Tooling/Syntax/TokensTest.cpp Index: clang/unittests/Tooling/Syntax/TokensTest.cpp =================================================================== --- clang/unittests/Tooling/Syntax/TokensTest.cpp +++ clang/unittests/Tooling/Syntax/TokensTest.cpp @@ -497,11 +497,28 @@ mappings: ['#'_0, 'int'_7) => ['int'_0, 'int'_0) ['FOO'_10, '<eof>'_11) => ['10'_3, '<eof>'_7) -)"}}; +)"}, + {R"cpp( + #define NUM 42 + #define ID(a) a + #define M 1 + ID + M(NUM) + )cpp", + R"(expanded tokens: + 1 + 42 +file './input.cpp' + spelled tokens: + # define NUM 42 # define ID ( a ) a # define M 1 + ID M ( NUM ) + mappings: + ['#'_0, 'M'_17) => ['1'_0, '1'_0) + ['M'_17, '<eof>'_21) => ['1'_0, '<eof>'_3) +)"}, + }; - for (auto &Test : TestCases) - EXPECT_EQ(Test.second, collectAndDump(Test.first)) - << collectAndDump(Test.first); + for (auto &Test : TestCases) { + std::string Dump = collectAndDump(Test.first); + EXPECT_EQ(Test.second, Dump) << Dump; + } } TEST_F(TokenCollectorTest, SpecialTokens) { Index: clang/lib/Tooling/Syntax/Tokens.cpp =================================================================== --- clang/lib/Tooling/Syntax/Tokens.cpp +++ clang/lib/Tooling/Syntax/Tokens.cpp @@ -436,14 +436,37 @@ SourceRange Range, const MacroArgs *Args) override { if (!Collector) return; - // Only record top-level expansions, not those where: + const auto &SM = Collector->PP.getSourceManager(); + // Only record top-level expansions that directly produce expanded tokens. + // This excludes those where: // - the macro use is inside a macro body, // - the macro appears in an argument to another macro. - if (!MacroNameTok.getLocation().isFileID() || - (LastExpansionEnd.isValid() && - Collector->PP.getSourceManager().isBeforeInTranslationUnit( - Range.getBegin(), LastExpansionEnd))) + // However macro expansion isn't really a tree, it's token rewrite rules, + // so there are other cases, e.g. + // #define B(X) X + // #define A 1 + B + // A(2) + // Both A and B produce expanded tokens, though the macro name 'B' comes + // from an expansion. The best we can do is merge the mappings for both. + + // The *last* token of the macro reference is in the main file for A and B. + if (Range.getEnd().isMacroID()) return; + // If there's a current expansion that encloses this one, this one can't be + // top-level. + if (LastExpansionEnd.isValid() && + !SM.isBeforeInTranslationUnit(LastExpansionEnd, Range.getEnd())) + return; + + // If the macro invocation B starts in a macro A but ends in a file, we'll + // create a merged mapping for A & B by overwriting the endpoint for A's + // startpoint. + if (!Range.getBegin().isFileID()) { + Range.setBegin(SM.getExpansionLoc(Range.getBegin())); + assert(Collector->Expansions.count(Range.getBegin().getRawEncoding()) && + "Overlapping macros should have same expansion location"); + } + Collector->Expansions[Range.getBegin().getRawEncoding()] = Range.getEnd(); LastExpansionEnd = Range.getEnd(); }
Index: clang/unittests/Tooling/Syntax/TokensTest.cpp =================================================================== --- clang/unittests/Tooling/Syntax/TokensTest.cpp +++ clang/unittests/Tooling/Syntax/TokensTest.cpp @@ -497,11 +497,28 @@ mappings: ['#'_0, 'int'_7) => ['int'_0, 'int'_0) ['FOO'_10, '<eof>'_11) => ['10'_3, '<eof>'_7) -)"}}; +)"}, + {R"cpp( + #define NUM 42 + #define ID(a) a + #define M 1 + ID + M(NUM) + )cpp", + R"(expanded tokens: + 1 + 42 +file './input.cpp' + spelled tokens: + # define NUM 42 # define ID ( a ) a # define M 1 + ID M ( NUM ) + mappings: + ['#'_0, 'M'_17) => ['1'_0, '1'_0) + ['M'_17, '<eof>'_21) => ['1'_0, '<eof>'_3) +)"}, + }; - for (auto &Test : TestCases) - EXPECT_EQ(Test.second, collectAndDump(Test.first)) - << collectAndDump(Test.first); + for (auto &Test : TestCases) { + std::string Dump = collectAndDump(Test.first); + EXPECT_EQ(Test.second, Dump) << Dump; + } } TEST_F(TokenCollectorTest, SpecialTokens) { Index: clang/lib/Tooling/Syntax/Tokens.cpp =================================================================== --- clang/lib/Tooling/Syntax/Tokens.cpp +++ clang/lib/Tooling/Syntax/Tokens.cpp @@ -436,14 +436,37 @@ SourceRange Range, const MacroArgs *Args) override { if (!Collector) return; - // Only record top-level expansions, not those where: + const auto &SM = Collector->PP.getSourceManager(); + // Only record top-level expansions that directly produce expanded tokens. + // This excludes those where: // - the macro use is inside a macro body, // - the macro appears in an argument to another macro. - if (!MacroNameTok.getLocation().isFileID() || - (LastExpansionEnd.isValid() && - Collector->PP.getSourceManager().isBeforeInTranslationUnit( - Range.getBegin(), LastExpansionEnd))) + // However macro expansion isn't really a tree, it's token rewrite rules, + // so there are other cases, e.g. + // #define B(X) X + // #define A 1 + B + // A(2) + // Both A and B produce expanded tokens, though the macro name 'B' comes + // from an expansion. The best we can do is merge the mappings for both. + + // The *last* token of the macro reference is in the main file for A and B. + if (Range.getEnd().isMacroID()) return; + // If there's a current expansion that encloses this one, this one can't be + // top-level. + if (LastExpansionEnd.isValid() && + !SM.isBeforeInTranslationUnit(LastExpansionEnd, Range.getEnd())) + return; + + // If the macro invocation B starts in a macro A but ends in a file, we'll + // create a merged mapping for A & B by overwriting the endpoint for A's + // startpoint. + if (!Range.getBegin().isFileID()) { + Range.setBegin(SM.getExpansionLoc(Range.getBegin())); + assert(Collector->Expansions.count(Range.getBegin().getRawEncoding()) && + "Overlapping macros should have same expansion location"); + } + Collector->Expansions[Range.getBegin().getRawEncoding()] = Range.getEnd(); LastExpansionEnd = Range.getEnd(); }
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits