ymandel updated this revision to Diff 197316.
ymandel marked 2 inline comments as done.
ymandel added a comment.
Addresses comments, including error handling style and signature of
ChangeConsumer.
Updates testing code to use new ChangeConsumer signature.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D61015/new/
https://reviews.llvm.org/D61015
Files:
clang/include/clang/Tooling/Refactoring/Transformer.h
clang/lib/Tooling/Refactoring/Transformer.cpp
clang/unittests/Tooling/TransformerTest.cpp
Index: clang/unittests/Tooling/TransformerTest.cpp
===================================================================
--- clang/unittests/Tooling/TransformerTest.cpp
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -10,6 +10,8 @@
#include "clang/ASTMatchers/ASTMatchers.h"
#include "clang/Tooling/Tooling.h"
+#include "llvm/Support/Errc.h"
+#include "llvm/Support/Error.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
@@ -18,6 +20,8 @@
using namespace ast_matchers;
namespace {
+using ::testing::IsEmpty;
+
constexpr char KHeaderContents[] = R"cc(
struct string {
string(const char*);
@@ -84,26 +88,43 @@
Factory->create(), Code, std::vector<std::string>(), "input.cc",
"clang-tool", std::make_shared<PCHContainerOperations>(),
FileContents)) {
+ llvm::errs() << "Running tool failed.\n";
+ return None;
+ }
+ if (ErrorCount != 0) {
+ llvm::errs() << "Generating changes failed.\n";
return None;
}
- auto ChangedCodeOrErr =
+ auto ChangedCode =
applyAtomicChanges("input.cc", Code, Changes, ApplyChangesSpec());
- if (auto Err = ChangedCodeOrErr.takeError()) {
- llvm::errs() << "Change failed: " << llvm::toString(std::move(Err))
- << "\n";
+ if (!ChangedCode) {
+ llvm::errs() << "Applying changes failed: "
+ << llvm::toString(ChangedCode.takeError()) << "\n";
return None;
}
- return *ChangedCodeOrErr;
+ return *ChangedCode;
+ }
+
+ Transformer::ChangeConsumer consumer() {
+ return [this](Expected<AtomicChange> C) {
+ if (C) {
+ Changes.push_back(std::move(*C));
+ } else {
+ consumeError(C.takeError());
+ ++ErrorCount;
+ }
+ };
}
void testRule(RewriteRule Rule, StringRef Input, StringRef Expected) {
- Transformer T(std::move(Rule),
- [this](const AtomicChange &C) { Changes.push_back(C); });
+ Transformer T(std::move(Rule), consumer());
T.registerMatchers(&MatchFinder);
compareSnippets(Expected, rewrite(Input));
}
clang::ast_matchers::MatchFinder MatchFinder;
+ // Records whether any errors occurred in individual changes.
+ int ErrorCount = 0;
AtomicChanges Changes;
private:
@@ -356,6 +377,23 @@
// Negative tests (where we expect no transformation to occur).
//
+// Tests for a conflict in edits from a single match for a rule.
+TEST_F(TransformerTest, TextGeneratorFailure) {
+ std::string Input = "int conflictOneRule() { return 3 + 7; }";
+ // Try to change the whole binary-operator expression AND one its operands:
+ StringRef O = "O";
+ auto AlwaysFail = [](const ast_matchers::MatchFinder::MatchResult &)
+ -> llvm::Expected<std::string> {
+ return llvm::createStringError(llvm::errc::invalid_argument, "ERROR");
+ };
+ Transformer T(makeRule(binaryOperator().bind(O), change<Expr>(O, AlwaysFail)),
+ consumer());
+ T.registerMatchers(&MatchFinder);
+ EXPECT_FALSE(rewrite(Input));
+ EXPECT_THAT(Changes, IsEmpty());
+ EXPECT_EQ(ErrorCount, 1);
+}
+
// Tests for a conflict in edits from a single match for a rule.
TEST_F(TransformerTest, OverlappingEditsInRule) {
std::string Input = "int conflictOneRule() { return 3 + 7; }";
@@ -364,13 +402,11 @@
Transformer T(
makeRule(binaryOperator(hasLHS(expr().bind(L))).bind(O),
{change<Expr>(O, "DELETE_OP"), change<Expr>(L, "DELETE_LHS")}),
- [this](const AtomicChange &C) { Changes.push_back(C); });
+ consumer());
T.registerMatchers(&MatchFinder);
- // The rewrite process fails...
- EXPECT_TRUE(rewrite(Input));
- // ... but one AtomicChange was consumed:
- ASSERT_EQ(Changes.size(), 1u);
- EXPECT_TRUE(Changes[0].hasError());
+ EXPECT_FALSE(rewrite(Input));
+ EXPECT_THAT(Changes, IsEmpty());
+ EXPECT_EQ(ErrorCount, 1);
}
// Tests for a conflict in edits across multiple matches (of the same rule).
@@ -379,27 +415,27 @@
// Try to change the whole binary-operator expression AND one its operands:
StringRef E = "E";
Transformer T(makeRule(expr().bind(E), change<Expr>(E, "DELETE_EXPR")),
- [this](const AtomicChange &C) { Changes.push_back(C); });
+ consumer());
T.registerMatchers(&MatchFinder);
// The rewrite process fails because the changes conflict with each other...
EXPECT_FALSE(rewrite(Input));
- // ... but all changes are (individually) fine:
- ASSERT_EQ(Changes.size(), 2u);
- EXPECT_FALSE(Changes[0].hasError());
- EXPECT_FALSE(Changes[1].hasError());
+ // ... but two changes were produced.
+ EXPECT_EQ(Changes.size(), 2u);
+ EXPECT_EQ(ErrorCount, 0);
}
TEST_F(TransformerTest, ErrorOccurredMatchSkipped) {
// Syntax error in the function body:
std::string Input = "void errorOccurred() { 3 }";
- Transformer T(
- makeRule(functionDecl(hasName("errorOccurred")), change<Decl>("DELETED;")),
- [this](const AtomicChange &C) { Changes.push_back(C); });
+ Transformer T(makeRule(functionDecl(hasName("errorOccurred")),
+ change<Decl>("DELETED;")),
+ consumer());
T.registerMatchers(&MatchFinder);
// The rewrite process itself fails...
EXPECT_FALSE(rewrite(Input));
- // ... and no changes are produced in the process.
- EXPECT_THAT(Changes, ::testing::IsEmpty());
+ // ... and no changes or errors are produced in the process.
+ EXPECT_THAT(Changes, IsEmpty());
+ EXPECT_EQ(ErrorCount, 0);
}
TEST_F(TransformerTest, NoTransformationInMacro) {
Index: clang/lib/Tooling/Refactoring/Transformer.cpp
===================================================================
--- clang/lib/Tooling/Refactoring/Transformer.cpp
+++ clang/lib/Tooling/Refactoring/Transformer.cpp
@@ -153,16 +153,19 @@
auto It = NodesMap.find(Edit.Target);
assert(It != NodesMap.end() && "Edit target must be bound in the match.");
- Expected<CharSourceRange> RangeOrErr = getTargetRange(
+ Expected<CharSourceRange> Range = getTargetRange(
Edit.Target, It->second, Edit.Kind, Edit.Part, *Result.Context);
- if (auto Err = RangeOrErr.takeError())
- return std::move(Err);
- Transformation T;
- T.Range = *RangeOrErr;
- if (T.Range.isInvalid() ||
- isOriginMacroBody(*Result.SourceManager, T.Range.getBegin()))
+ if (!Range)
+ return Range.takeError();
+ if (Range->isInvalid() ||
+ isOriginMacroBody(*Result.SourceManager, Range->getBegin()))
return SmallVector<Transformation, 0>();
- T.Replacement = Edit.Replacement(Result);
+ auto Replacement = Edit.Replacement(Result);
+ if (!Replacement)
+ return Replacement.takeError();
+ Transformation T;
+ T.Range = *Range;
+ T.Replacement = std::move(*Replacement);
Transformations.push_back(std::move(T));
}
return Transformations;
@@ -194,14 +197,13 @@
Root->second.getSourceRange().getBegin());
assert(RootLoc.isValid() && "Invalid location for Root node of match.");
- auto TransformationsOrErr = translateEdits(Result, Rule.Edits);
- if (auto Err = TransformationsOrErr.takeError()) {
- llvm::errs() << "Transformation failed: " << llvm::toString(std::move(Err))
- << "\n";
+ auto Transformations = translateEdits(Result, Rule.Edits);
+ if (!Transformations) {
+ Consumer(Transformations.takeError());
return;
}
- auto &Transformations = *TransformationsOrErr;
- if (Transformations.empty()) {
+
+ if (Transformations->empty()) {
// No rewrite applied (but no error encountered either).
RootLoc.print(llvm::errs() << "note: skipping match at loc ",
*Result.SourceManager);
@@ -209,14 +211,14 @@
return;
}
- // Convert the result to an AtomicChange.
+ // Record the results in the AtomicChange.
AtomicChange AC(*Result.SourceManager, RootLoc);
- for (const auto &T : Transformations) {
+ for (const auto &T : *Transformations) {
if (auto Err = AC.replace(*Result.SourceManager, T.Range, T.Replacement)) {
- AC.setError(llvm::toString(std::move(Err)));
- break;
+ Consumer(std::move(Err));
+ return;
}
}
- Consumer(AC);
+ Consumer(std::move(AC));
}
Index: clang/include/clang/Tooling/Refactoring/Transformer.h
===================================================================
--- clang/include/clang/Tooling/Refactoring/Transformer.h
+++ clang/include/clang/Tooling/Refactoring/Transformer.h
@@ -44,12 +44,16 @@
Name,
};
-using TextGenerator =
- std::function<std::string(const ast_matchers::MatchFinder::MatchResult &)>;
+// Note that \p TextGenerator is allowed to fail, e.g. when trying to access a
+// matched node that was not bound. Allowing this to fail simplifies error
+// handling for interactive tools like clang-query.
+using TextGenerator = std::function<Expected<std::string>(
+ const ast_matchers::MatchFinder::MatchResult &)>;
/// Wraps a string as a TextGenerator.
inline TextGenerator text(std::string M) {
- return [M](const ast_matchers::MatchFinder::MatchResult &) { return M; };
+ return [M](const ast_matchers::MatchFinder::MatchResult &)
+ -> Expected<std::string> { return M; };
}
// Description of a source-code edit, expressed in terms of an AST node.
@@ -222,11 +226,13 @@
class Transformer : public ast_matchers::MatchFinder::MatchCallback {
public:
using ChangeConsumer =
- std::function<void(const clang::tooling::AtomicChange &Change)>;
+ std::function<void(Expected<clang::tooling::AtomicChange> Change)>;
- /// \param Consumer Receives each successful rewrite as an \c AtomicChange.
- /// Note that clients are responsible for handling the case that independent
- /// \c AtomicChanges conflict with each other.
+ /// \param Consumer Receives each rewrite or error. Will not necessarily be
+ /// called for each match; for example, if the rewrite is not applicable
+ /// because of macros, but doesn't fail. Note that clients are responsible
+ /// for handling the case that independent \c AtomicChanges conflict with each
+ /// other.
Transformer(RewriteRule Rule, ChangeConsumer Consumer)
: Rule(std::move(Rule)), Consumer(std::move(Consumer)) {}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
