This revision was automatically updated to reflect the committed changes.
Closed by commit rGb9d2bf38e86e: [libTooling] Fix bug in Stencil handling of
macro ranges (authored by ymandel).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72274/new/
https://reviews.llvm.org/D72274
Files:
clang/include/clang/Tooling/Transformer/SourceCode.h
clang/lib/Tooling/Transformer/SourceCode.cpp
clang/lib/Tooling/Transformer/Stencil.cpp
clang/unittests/Tooling/SourceCodeTest.cpp
clang/unittests/Tooling/StencilTest.cpp
Index: clang/unittests/Tooling/StencilTest.cpp
===================================================================
--- clang/unittests/Tooling/StencilTest.cpp
+++ clang/unittests/Tooling/StencilTest.cpp
@@ -368,6 +368,21 @@
testExpr(Id, "3;", run(SimpleFn), "Bound");
}
+TEST_F(StencilTest, CatOfInvalidRangeFails) {
+ StringRef Snippet = R"cpp(
+#define MACRO (3.77)
+ double foo(double d);
+ foo(MACRO);)cpp";
+
+ auto StmtMatch =
+ matchStmt(Snippet, callExpr(callee(functionDecl(hasName("foo"))),
+ argumentCountIs(1),
+ hasArgument(0, expr().bind("arg"))));
+ ASSERT_TRUE(StmtMatch);
+ Stencil S = cat(node("arg"));
+ EXPECT_THAT_EXPECTED(S->eval(StmtMatch->Result), Failed<StringError>());
+}
+
TEST(StencilToStringTest, RawTextOp) {
auto S = cat("foo bar baz");
StringRef Expected = R"("foo bar baz")";
Index: clang/unittests/Tooling/SourceCodeTest.cpp
===================================================================
--- clang/unittests/Tooling/SourceCodeTest.cpp
+++ clang/unittests/Tooling/SourceCodeTest.cpp
@@ -10,16 +10,20 @@
#include "TestVisitor.h"
#include "clang/Basic/Diagnostic.h"
#include "llvm/Testing/Support/Annotations.h"
+#include "llvm/Testing/Support/Error.h"
#include "llvm/Testing/Support/SupportHelpers.h"
#include <gmock/gmock.h>
#include <gtest/gtest.h>
using namespace clang;
+using llvm::Failed;
+using llvm::Succeeded;
using llvm::ValueIs;
using tooling::getExtendedText;
using tooling::getRangeForEdit;
using tooling::getText;
+using tooling::validateEditRange;
namespace {
@@ -200,4 +204,116 @@
Visitor.runOver(Code.code());
}
+TEST(SourceCodeTest, EditRangeWithMacroExpansionsIsValid) {
+ // The call expression, whose range we are extracting, includes two macro
+ // expansions.
+ llvm::StringRef Code = R"cpp(
+#define M(a) a * 13
+int foo(int x, int y);
+int a = foo(M(1), M(2));
+)cpp";
+
+ CallsVisitor Visitor;
+
+ Visitor.OnCall = [](CallExpr *CE, ASTContext *Context) {
+ auto Range = CharSourceRange::getTokenRange(CE->getSourceRange());
+ EXPECT_THAT_ERROR(validateEditRange(Range, Context->getSourceManager()),
+ Succeeded());
+ };
+ Visitor.runOver(Code);
+}
+
+TEST(SourceCodeTest, SpellingRangeOfMacroArgIsValid) {
+ llvm::StringRef Code = R"cpp(
+#define FOO(a) a + 7.0;
+int a = FOO(10);
+)cpp";
+
+ IntLitVisitor Visitor;
+ Visitor.OnIntLit = [](IntegerLiteral *Expr, ASTContext *Context) {
+ SourceLocation ArgLoc =
+ Context->getSourceManager().getSpellingLoc(Expr->getBeginLoc());
+ // The integer literal is a single token.
+ auto ArgRange = CharSourceRange::getTokenRange(ArgLoc);
+ EXPECT_THAT_ERROR(validateEditRange(ArgRange, Context->getSourceManager()),
+ Succeeded());
+ };
+ Visitor.runOver(Code);
+}
+
+TEST(SourceCodeTest, InvalidEditRangeIsInvalid) {
+ llvm::StringRef Code = "int c = 10;";
+
+ // We use the visitor just to get a valid context.
+ IntLitVisitor Visitor;
+ Visitor.OnIntLit = [](IntegerLiteral *, ASTContext *Context) {
+ CharSourceRange Invalid;
+ EXPECT_THAT_ERROR(validateEditRange(Invalid, Context->getSourceManager()),
+ Failed());
+ };
+ Visitor.runOver(Code);
+}
+
+TEST(SourceCodeTest, InvertedEditRangeIsInvalid) {
+ llvm::StringRef Code = R"cpp(
+int foo(int x);
+int a = foo(2);
+)cpp";
+
+ CallsVisitor Visitor;
+ Visitor.OnCall = [](CallExpr *Expr, ASTContext *Context) {
+ auto InvertedRange = CharSourceRange::getTokenRange(
+ SourceRange(Expr->getEndLoc(), Expr->getBeginLoc()));
+ EXPECT_THAT_ERROR(
+ validateEditRange(InvertedRange, Context->getSourceManager()),
+ Failed());
+ };
+ Visitor.runOver(Code);
+}
+
+TEST(SourceCodeTest, MacroArgIsInvalid) {
+ llvm::StringRef Code = R"cpp(
+#define FOO(a) a + 7.0;
+int a = FOO(10);
+)cpp";
+
+ IntLitVisitor Visitor;
+ Visitor.OnIntLit = [](IntegerLiteral *Expr, ASTContext *Context) {
+ auto Range = CharSourceRange::getTokenRange(Expr->getSourceRange());
+ EXPECT_THAT_ERROR(validateEditRange(Range, Context->getSourceManager()),
+ Failed());
+ };
+ Visitor.runOver(Code);
+}
+
+TEST(SourceCodeTest, EditWholeMacroExpansionIsInvalid) {
+ llvm::StringRef Code = R"cpp(
+#define FOO 10
+int a = FOO;
+)cpp";
+
+ IntLitVisitor Visitor;
+ Visitor.OnIntLit = [](IntegerLiteral *Expr, ASTContext *Context) {
+ auto Range = CharSourceRange::getTokenRange(Expr->getSourceRange());
+ EXPECT_THAT_ERROR(validateEditRange(Range, Context->getSourceManager()),
+ Failed());
+
+ };
+ Visitor.runOver(Code);
+}
+
+TEST(SourceCodeTest, EditPartialMacroExpansionIsInvalid) {
+ llvm::StringRef Code = R"cpp(
+#define BAR 10+
+int c = BAR 3.0;
+)cpp";
+
+ IntLitVisitor Visitor;
+ Visitor.OnIntLit = [](IntegerLiteral *Expr, ASTContext *Context) {
+ auto Range = CharSourceRange::getTokenRange(Expr->getSourceRange());
+ EXPECT_THAT_ERROR(validateEditRange(Range, Context->getSourceManager()),
+ Failed());
+ };
+ Visitor.runOver(Code);
+}
} // end anonymous namespace
Index: clang/lib/Tooling/Transformer/Stencil.cpp
===================================================================
--- clang/lib/Tooling/Transformer/Stencil.cpp
+++ clang/lib/Tooling/Transformer/Stencil.cpp
@@ -230,6 +230,8 @@
auto Range = Data.Selector(Match);
if (!Range)
return Range.takeError();
+ if (auto Err = tooling::validateEditRange(*Range, *Match.SourceManager))
+ return Err;
*Result += tooling::getText(*Range, *Match.Context);
return Error::success();
}
Index: clang/lib/Tooling/Transformer/SourceCode.cpp
===================================================================
--- clang/lib/Tooling/Transformer/SourceCode.cpp
+++ clang/lib/Tooling/Transformer/SourceCode.cpp
@@ -11,9 +11,13 @@
//===----------------------------------------------------------------------===//
#include "clang/Tooling/Transformer/SourceCode.h"
#include "clang/Lex/Lexer.h"
+#include "llvm/Support/Errc.h"
using namespace clang;
+using llvm::errc;
+using llvm::StringError;
+
StringRef clang::tooling::getText(CharSourceRange Range,
const ASTContext &Context) {
return Lexer::getSourceText(Range, Context.getSourceManager(),
@@ -30,6 +34,34 @@
return CharSourceRange::getTokenRange(Range.getBegin(), Tok->getLocation());
}
+llvm::Error clang::tooling::validateEditRange(const CharSourceRange &Range,
+ const SourceManager &SM) {
+ if (Range.isInvalid())
+ return llvm::make_error<StringError>(errc::invalid_argument,
+ "Invalid range");
+
+ if (Range.getBegin().isMacroID() || Range.getEnd().isMacroID())
+ return llvm::make_error<StringError>(
+ errc::invalid_argument, "Range starts or ends in a macro expansion");
+
+ if (SM.isInSystemHeader(Range.getBegin()) ||
+ SM.isInSystemHeader(Range.getEnd()))
+ return llvm::make_error<StringError>(errc::invalid_argument,
+ "Range is in system header");
+
+ std::pair<FileID, unsigned> BeginInfo = SM.getDecomposedLoc(Range.getBegin());
+ std::pair<FileID, unsigned> EndInfo = SM.getDecomposedLoc(Range.getEnd());
+ if (BeginInfo.first != EndInfo.first)
+ return llvm::make_error<StringError>(
+ errc::invalid_argument, "Range begins and ends in different files");
+
+ if (BeginInfo.second > EndInfo.second)
+ return llvm::make_error<StringError>(
+ errc::invalid_argument, "Range's begin is past its end");
+
+ return llvm::Error::success();
+}
+
llvm::Optional<CharSourceRange>
clang::tooling::getRangeForEdit(const CharSourceRange &EditRange,
const SourceManager &SM,
@@ -46,20 +78,9 @@
// foo(DO_NOTHING(6))
// Decide whether the current behavior is desirable and modify if not.
CharSourceRange Range = Lexer::makeFileCharRange(EditRange, SM, LangOpts);
- if (Range.isInvalid())
- return None;
-
- if (Range.getBegin().isMacroID() || Range.getEnd().isMacroID())
- return None;
- if (SM.isInSystemHeader(Range.getBegin()) ||
- SM.isInSystemHeader(Range.getEnd()))
- return None;
-
- std::pair<FileID, unsigned> BeginInfo = SM.getDecomposedLoc(Range.getBegin());
- std::pair<FileID, unsigned> EndInfo = SM.getDecomposedLoc(Range.getEnd());
- if (BeginInfo.first != EndInfo.first ||
- BeginInfo.second > EndInfo.second)
- return None;
-
+ bool IsInvalid = llvm::errorToBool(validateEditRange(Range, SM));
+ if (IsInvalid)
+ return llvm::None;
return Range;
+
}
Index: clang/include/clang/Tooling/Transformer/SourceCode.h
===================================================================
--- clang/include/clang/Tooling/Transformer/SourceCode.h
+++ clang/include/clang/Tooling/Transformer/SourceCode.h
@@ -73,13 +73,18 @@
return getText(getExtendedRange(Node, Next, Context), Context);
}
-// Attempts to resolve the given range to one that can be edited by a rewrite;
-// generally, one that starts and ends within a particular file. It supports
-// a limited set of cases involving source locations in macro expansions.
+/// Determines whether \p Range is one that can be edited by a rewrite;
+/// generally, one that starts and ends within a particular file.
+llvm::Error validateEditRange(const CharSourceRange &Range,
+ const SourceManager &SM);
+
+/// Attempts to resolve the given range to one that can be edited by a rewrite;
+/// generally, one that starts and ends within a particular file. It supports a
+/// limited set of cases involving source locations in macro expansions. If a
+/// value is returned, it satisfies \c validateEditRange.
llvm::Optional<CharSourceRange>
getRangeForEdit(const CharSourceRange &EditRange, const SourceManager &SM,
const LangOptions &LangOpts);
-
inline llvm::Optional<CharSourceRange>
getRangeForEdit(const CharSourceRange &EditRange, const ASTContext &Context) {
return getRangeForEdit(EditRange, Context.getSourceManager(),
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
