On Tue, Jan 15, 2019 at 12:23 PM Benjamin Kramer via cfe-commits <cfe-commits@lists.llvm.org> wrote: > > Author: d0k > Date: Tue Jan 15 09:20:05 2019 > New Revision: 351209 > > URL: http://llvm.org/viewvc/llvm-project?rev=351209&view=rev > Log: > Revert "Correct the source range returned from preprocessor callbacks." > > This reverts commit r350891. Also add a test case that would return an > empty string with r350891.
Thank you for this! I've fixed and commit in r351470. ~Aaron > > Modified: > cfe/trunk/include/clang/Lex/Preprocessor.h > cfe/trunk/lib/Lex/PPDirectives.cpp > cfe/trunk/lib/Lex/PPExpressions.cpp > cfe/trunk/unittests/Lex/PPCallbacksTest.cpp > > Modified: cfe/trunk/include/clang/Lex/Preprocessor.h > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Lex/Preprocessor.h?rev=351209&r1=351208&r2=351209&view=diff > ============================================================================== > --- cfe/trunk/include/clang/Lex/Preprocessor.h (original) > +++ cfe/trunk/include/clang/Lex/Preprocessor.h Tue Jan 15 09:20:05 2019 > @@ -1816,8 +1816,8 @@ public: > void CheckEndOfDirective(const char *DirType, bool EnableMacros = false); > > /// Read and discard all tokens remaining on the current line until > - /// the tok::eod token is found. Returns the range of the skipped tokens. > - SourceRange DiscardUntilEndOfDirective(); > + /// the tok::eod token is found. > + void DiscardUntilEndOfDirective(); > > /// Returns true if the preprocessor has seen a use of > /// __DATE__ or __TIME__ in the file so far. > @@ -1982,9 +1982,6 @@ private: > > /// True if the expression contained identifiers that were undefined. > bool IncludedUndefinedIds; > - > - /// The source range for the expression. > - SourceRange ExprRange; > }; > > /// Evaluate an integer constant expression that may occur after a > > Modified: cfe/trunk/lib/Lex/PPDirectives.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Lex/PPDirectives.cpp?rev=351209&r1=351208&r2=351209&view=diff > ============================================================================== > --- cfe/trunk/lib/Lex/PPDirectives.cpp (original) > +++ cfe/trunk/lib/Lex/PPDirectives.cpp Tue Jan 15 09:20:05 2019 > @@ -79,18 +79,12 @@ Preprocessor::AllocateVisibilityMacroDir > > /// Read and discard all tokens remaining on the current line until > /// the tok::eod token is found. > -SourceRange Preprocessor::DiscardUntilEndOfDirective() { > +void Preprocessor::DiscardUntilEndOfDirective() { > Token Tmp; > - SourceRange Res; > - > - LexUnexpandedToken(Tmp); > - Res.setBegin(Tmp.getLocation()); > - while (Tmp.isNot(tok::eod)) { > - assert(Tmp.isNot(tok::eof) && "EOF seen while discarding directive > tokens"); > + do { > LexUnexpandedToken(Tmp); > - } > - Res.setEnd(Tmp.getLocation()); > - return Res; > + assert(Tmp.isNot(tok::eof) && "EOF seen while discarding directive > tokens"); > + } while (Tmp.isNot(tok::eod)); > } > > /// Enumerates possible cases of #define/#undef a reserved identifier. > @@ -544,19 +538,19 @@ void Preprocessor::SkipExcludedCondition > if (CondInfo.WasSkipping || CondInfo.FoundNonSkip) { > DiscardUntilEndOfDirective(); > } else { > + const SourceLocation CondBegin = CurPPLexer->getSourceLocation(); > // Restore the value of LexingRawMode so that identifiers are > // looked up, etc, inside the #elif expression. > assert(CurPPLexer->LexingRawMode && "We have to be skipping > here!"); > CurPPLexer->LexingRawMode = false; > IdentifierInfo *IfNDefMacro = nullptr; > - DirectiveEvalResult DER = EvaluateDirectiveExpression(IfNDefMacro); > - const bool CondValue = DER.Conditional; > + const bool CondValue = > EvaluateDirectiveExpression(IfNDefMacro).Conditional; > CurPPLexer->LexingRawMode = true; > if (Callbacks) { > - Callbacks->Elif( > - Tok.getLocation(), DER.ExprRange, > - (CondValue ? PPCallbacks::CVK_True : PPCallbacks::CVK_False), > - CondInfo.IfLoc); > + const SourceLocation CondEnd = CurPPLexer->getSourceLocation(); > + Callbacks->Elif(Tok.getLocation(), > + SourceRange(CondBegin, CondEnd), > + (CondValue ? PPCallbacks::CVK_True : > PPCallbacks::CVK_False), CondInfo.IfLoc); > } > // If this condition is true, enter it! > if (CondValue) { > @@ -1122,24 +1116,19 @@ void Preprocessor::HandleLineDirective() > ; // ok > else if (StrTok.isNot(tok::string_literal)) { > Diag(StrTok, diag::err_pp_line_invalid_filename); > - DiscardUntilEndOfDirective(); > - return; > + return DiscardUntilEndOfDirective(); > } else if (StrTok.hasUDSuffix()) { > Diag(StrTok, diag::err_invalid_string_udl); > - DiscardUntilEndOfDirective(); > - return; > + return DiscardUntilEndOfDirective(); > } else { > // Parse and validate the string, converting it into a unique ID. > StringLiteralParser Literal(StrTok, *this); > assert(Literal.isAscii() && "Didn't allow wide strings in"); > - if (Literal.hadError) { > - DiscardUntilEndOfDirective(); > - return; > - } > + if (Literal.hadError) > + return DiscardUntilEndOfDirective(); > if (Literal.Pascal) { > Diag(StrTok, diag::err_pp_linemarker_invalid_filename); > - DiscardUntilEndOfDirective(); > - return; > + return DiscardUntilEndOfDirective(); > } > FilenameID = SourceMgr.getLineTableFilenameID(Literal.GetString()); > > @@ -1272,24 +1261,19 @@ void Preprocessor::HandleDigitDirective( > FileKind = SourceMgr.getFileCharacteristic(DigitTok.getLocation()); > } else if (StrTok.isNot(tok::string_literal)) { > Diag(StrTok, diag::err_pp_linemarker_invalid_filename); > - DiscardUntilEndOfDirective(); > - return; > + return DiscardUntilEndOfDirective(); > } else if (StrTok.hasUDSuffix()) { > Diag(StrTok, diag::err_invalid_string_udl); > - DiscardUntilEndOfDirective(); > - return; > + return DiscardUntilEndOfDirective(); > } else { > // Parse and validate the string, converting it into a unique ID. > StringLiteralParser Literal(StrTok, *this); > assert(Literal.isAscii() && "Didn't allow wide strings in"); > - if (Literal.hadError) { > - DiscardUntilEndOfDirective(); > - return; > - } > + if (Literal.hadError) > + return DiscardUntilEndOfDirective(); > if (Literal.Pascal) { > Diag(StrTok, diag::err_pp_linemarker_invalid_filename); > - DiscardUntilEndOfDirective(); > - return; > + return DiscardUntilEndOfDirective(); > } > FilenameID = SourceMgr.getLineTableFilenameID(Literal.GetString()); > > @@ -1359,8 +1343,7 @@ void Preprocessor::HandleIdentSCCSDirect > > if (StrTok.hasUDSuffix()) { > Diag(StrTok, diag::err_invalid_string_udl); > - DiscardUntilEndOfDirective(); > - return; > + return DiscardUntilEndOfDirective(); > } > > // Verify that there is nothing after the string, other than EOD. > @@ -2800,8 +2783,10 @@ void Preprocessor::HandleIfDirective(Tok > > // Parse and evaluate the conditional expression. > IdentifierInfo *IfNDefMacro = nullptr; > + const SourceLocation ConditionalBegin = CurPPLexer->getSourceLocation(); > const DirectiveEvalResult DER = EvaluateDirectiveExpression(IfNDefMacro); > const bool ConditionalTrue = DER.Conditional; > + const SourceLocation ConditionalEnd = CurPPLexer->getSourceLocation(); > > // If this condition is equivalent to #ifndef X, and if this is the first > // directive seen, handle it for the multiple-include optimization. > @@ -2814,9 +2799,9 @@ void Preprocessor::HandleIfDirective(Tok > } > > if (Callbacks) > - Callbacks->If( > - IfToken.getLocation(), DER.ExprRange, > - (ConditionalTrue ? PPCallbacks::CVK_True : PPCallbacks::CVK_False)); > + Callbacks->If(IfToken.getLocation(), > + SourceRange(ConditionalBegin, ConditionalEnd), > + (ConditionalTrue ? PPCallbacks::CVK_True : > PPCallbacks::CVK_False)); > > // Should we include the stuff contained by this directive? > if (PPOpts->SingleFileParseMode && DER.IncludedUndefinedIds) { > @@ -2909,7 +2894,9 @@ void Preprocessor::HandleElifDirective(T > // #elif directive in a non-skipping conditional... start skipping. > // We don't care what the condition is, because we will always skip it > (since > // the block immediately before it was included). > - SourceRange ConditionRange = DiscardUntilEndOfDirective(); > + const SourceLocation ConditionalBegin = CurPPLexer->getSourceLocation(); > + DiscardUntilEndOfDirective(); > + const SourceLocation ConditionalEnd = CurPPLexer->getSourceLocation(); > > PPConditionalInfo CI; > if (CurPPLexer->popConditionalLevel(CI)) { > @@ -2925,7 +2912,8 @@ void Preprocessor::HandleElifDirective(T > if (CI.FoundElse) Diag(ElifToken, diag::pp_err_elif_after_else); > > if (Callbacks) > - Callbacks->Elif(ElifToken.getLocation(), ConditionRange, > + Callbacks->Elif(ElifToken.getLocation(), > + SourceRange(ConditionalBegin, ConditionalEnd), > PPCallbacks::CVK_NotEvaluated, CI.IfLoc); > > if (PPOpts->SingleFileParseMode && !CI.FoundNonSkip) { > > Modified: cfe/trunk/lib/Lex/PPExpressions.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Lex/PPExpressions.cpp?rev=351209&r1=351208&r2=351209&view=diff > ============================================================================== > --- cfe/trunk/lib/Lex/PPExpressions.cpp (original) > +++ cfe/trunk/lib/Lex/PPExpressions.cpp Tue Jan 15 09:20:05 2019 > @@ -152,8 +152,8 @@ static bool EvaluateDefined(PPValue &Res > return true; > } > // Consume the ). > - PP.LexNonComment(PeekTok); > Result.setEnd(PeekTok.getLocation()); > + PP.LexNonComment(PeekTok); > } else { > // Consume identifier. > Result.setEnd(PeekTok.getLocation()); > @@ -849,7 +849,7 @@ Preprocessor::EvaluateDirectiveExpressio > > // Restore 'DisableMacroExpansion'. > DisableMacroExpansion = DisableMacroExpansionAtStartOfDirective; > - return {false, DT.IncludedUndefinedIds, {}}; > + return {false, DT.IncludedUndefinedIds}; > } > > // If we are at the end of the expression after just parsing a value, there > @@ -863,7 +863,7 @@ Preprocessor::EvaluateDirectiveExpressio > > // Restore 'DisableMacroExpansion'. > DisableMacroExpansion = DisableMacroExpansionAtStartOfDirective; > - return {ResVal.Val != 0, DT.IncludedUndefinedIds, ResVal.getRange()}; > + return {ResVal.Val != 0, DT.IncludedUndefinedIds}; > } > > // Otherwise, we must have a binary operator (e.g. "#if 1 < 2"), so parse > the > @@ -876,7 +876,7 @@ Preprocessor::EvaluateDirectiveExpressio > > // Restore 'DisableMacroExpansion'. > DisableMacroExpansion = DisableMacroExpansionAtStartOfDirective; > - return {false, DT.IncludedUndefinedIds, ResVal.getRange()}; > + return {false, DT.IncludedUndefinedIds}; > } > > // If we aren't at the tok::eod token, something bad happened, like an > extra > @@ -888,5 +888,5 @@ Preprocessor::EvaluateDirectiveExpressio > > // Restore 'DisableMacroExpansion'. > DisableMacroExpansion = DisableMacroExpansionAtStartOfDirective; > - return {ResVal.Val != 0, DT.IncludedUndefinedIds, ResVal.getRange()}; > + return {ResVal.Val != 0, DT.IncludedUndefinedIds}; > } > > Modified: cfe/trunk/unittests/Lex/PPCallbacksTest.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Lex/PPCallbacksTest.cpp?rev=351209&r1=351208&r2=351209&view=diff > ============================================================================== > --- cfe/trunk/unittests/Lex/PPCallbacksTest.cpp (original) > +++ cfe/trunk/unittests/Lex/PPCallbacksTest.cpp Tue Jan 15 09:20:05 2019 > @@ -431,58 +431,16 @@ TEST_F(PPCallbacksTest, OpenCLExtensionP > } > > TEST_F(PPCallbacksTest, DirectiveExprRanges) { > - const auto &Results1 = DirectiveExprRange("#if FLUZZY_FLOOF\n#endif\n"); > - EXPECT_EQ(Results1.size(), 1U); > - EXPECT_EQ( > - GetSourceStringToEnd(CharSourceRange(Results1[0].ConditionRange, > false)), > - "FLUZZY_FLOOF"); > - > - const auto &Results2 = DirectiveExprRange("#if 1 + 4 < 7\n#endif\n"); > - EXPECT_EQ(Results2.size(), 1U); > - EXPECT_EQ( > - GetSourceStringToEnd(CharSourceRange(Results2[0].ConditionRange, > false)), > - "1 + 4 < 7"); > - > - const auto &Results3 = DirectiveExprRange("#if 1 + \\\n 2\n#endif\n"); > - EXPECT_EQ(Results3.size(), 1U); > - EXPECT_EQ( > - GetSourceStringToEnd(CharSourceRange(Results3[0].ConditionRange, > false)), > - "1 + \\\n 2"); > - > - const auto &Results4 = DirectiveExprRange("#if 0\n#elif FLOOFY\n#endif\n"); > - EXPECT_EQ(Results4.size(), 2U); > - EXPECT_EQ( > - GetSourceStringToEnd(CharSourceRange(Results4[0].ConditionRange, > false)), > - "0"); > - EXPECT_EQ( > - GetSourceStringToEnd(CharSourceRange(Results4[1].ConditionRange, > false)), > - "FLOOFY"); > - > - const auto &Results5 = DirectiveExprRange("#if 1\n#elif FLOOFY\n#endif\n"); > - EXPECT_EQ(Results5.size(), 2U); > - EXPECT_EQ( > - GetSourceStringToEnd(CharSourceRange(Results5[0].ConditionRange, > false)), > - "1"); > - EXPECT_EQ( > - GetSourceStringToEnd(CharSourceRange(Results5[1].ConditionRange, > false)), > - "FLOOFY"); > - > - const auto &Results6 = > - DirectiveExprRange("#if defined(FLUZZY_FLOOF)\n#endif\n"); > - EXPECT_EQ(Results6.size(), 1U); > - EXPECT_EQ( > - GetSourceStringToEnd(CharSourceRange(Results6[0].ConditionRange, > false)), > - "defined(FLUZZY_FLOOF)"); > - > - const auto &Results7 = > - DirectiveExprRange("#if 1\n#elif defined(FLOOFY)\n#endif\n"); > - EXPECT_EQ(Results7.size(), 2U); > - EXPECT_EQ( > - GetSourceStringToEnd(CharSourceRange(Results7[0].ConditionRange, > false)), > - "1"); > - EXPECT_EQ( > - GetSourceStringToEnd(CharSourceRange(Results7[1].ConditionRange, > false)), > - "defined(FLOOFY)"); > + const auto &Results8 = > + DirectiveExprRange("#define FLOOFY 0\n#if __FILE__ > > FLOOFY\n#endif\n"); > + EXPECT_EQ(Results8.size(), 1U); > + EXPECT_EQ( > + GetSourceStringToEnd(CharSourceRange(Results8[0].ConditionRange, > false)), > + " __FILE__ > FLOOFY\n#"); > + EXPECT_EQ( > + Lexer::getSourceText(CharSourceRange(Results8[0].ConditionRange, > false), > + SourceMgr, LangOpts), > + " __FILE__ > FLOOFY\n"); > } > > } // namespace > > > _______________________________________________ > cfe-commits mailing list > cfe-commits@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
