[clang] 5e5d366 - [libTooling] Simplify the representation of Transformer's RewriteRules.
Yitzhak Mandelbaum via cfe-commits
cfe-commits at lists.llvm.org
Wed Apr 8 05:47:04 PDT 2020
Author: Yitzhak Mandelbaum
Date: 2020-04-08T08:45:41-04:00
New Revision: 5e5d36671833e186e847e74bb5ed0c559625b906
URL: https://github.com/llvm/llvm-project/commit/5e5d36671833e186e847e74bb5ed0c559625b906
DIFF: https://github.com/llvm/llvm-project/commit/5e5d36671833e186e847e74bb5ed0c559625b906.diff
LOG: [libTooling] Simplify the representation of Transformer's RewriteRules.
Summary:
This revision simplifies the representation of edits in rewrite rules. The
simplified form is more general, allowing the user more flexibility in building
custom edit specifications.
The changes extend the API, without changing the signature of existing
functions. So this only risks breaking users that directly accessed the
`RewriteRule` struct.
Reviewers: gribozavr2
Subscribers: jfb, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D77419
Added:
Modified:
clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
clang/include/clang/Tooling/Transformer/RewriteRule.h
clang/lib/Tooling/Transformer/RewriteRule.cpp
clang/lib/Tooling/Transformer/Transformer.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp b/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
index 515997b14231..821c6290fe01 100644
--- a/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
+++ b/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
@@ -73,16 +73,15 @@ void TransformerClangTidyCheck::check(
assert(Rule && "check() should not fire if Rule is None");
RewriteRule::Case Case = transformer::detail::findSelectedCase(Result, *Rule);
- Expected<SmallVector<transformer::detail::Transformation, 1>>
- Transformations = transformer::detail::translateEdits(Result, Case.Edits);
- if (!Transformations) {
- llvm::errs() << "Rewrite failed: "
- << llvm::toString(Transformations.takeError()) << "\n";
+ Expected<SmallVector<transformer::Edit, 1>> Edits = Case.Edits(Result);
+ if (!Edits) {
+ llvm::errs() << "Rewrite failed: " << llvm::toString(Edits.takeError())
+ << "\n";
return;
}
// No rewrite applied, but no error encountered either.
- if (Transformations->empty())
+ if (Edits->empty())
return;
Expected<std::string> Explanation = Case.Explanation->eval(Result);
@@ -93,9 +92,8 @@ void TransformerClangTidyCheck::check(
}
// Associate the diagnostic with the location of the first change.
- DiagnosticBuilder Diag =
- diag((*Transformations)[0].Range.getBegin(), *Explanation);
- for (const auto &T : *Transformations)
+ DiagnosticBuilder Diag = diag((*Edits)[0].Range.getBegin(), *Explanation);
+ for (const auto &T : *Edits)
Diag << FixItHint::CreateReplacement(T.Range, T.Replacement);
for (const auto &I : Case.AddedIncludes) {
diff --git a/clang/include/clang/Tooling/Transformer/RewriteRule.h b/clang/include/clang/Tooling/Transformer/RewriteRule.h
index 4fca3bc9e2e8..4a5e5556cdff 100644
--- a/clang/include/clang/Tooling/Transformer/RewriteRule.h
+++ b/clang/include/clang/Tooling/Transformer/RewriteRule.h
@@ -30,6 +30,19 @@
namespace clang {
namespace transformer {
+/// A concrete description of a source edit, represented by a character range in
+/// the source to be replaced and a corresponding replacement string.
+struct Edit {
+ CharSourceRange Range;
+ std::string Replacement;
+};
+
+/// Maps a match result to a list of concrete edits (with possible
+/// failure). This type is a building block of rewrite rules, but users will
+/// generally work in terms of `ASTEdit`s (below) rather than directly in terms
+/// of `EditGenerator`.
+using EditGenerator = MatchConsumer<llvm::SmallVector<Edit, 1>>;
+
using TextGenerator = std::shared_ptr<MatchComputation<std::string>>;
// Description of a source-code edit, expressed in terms of an AST node.
@@ -74,6 +87,19 @@ struct ASTEdit {
TextGenerator Note;
};
+/// Lifts a list of `ASTEdit`s into an `EditGenerator`.
+///
+/// The `EditGenerator` will return an empty vector if any of the edits apply to
+/// portions of the source that are ineligible for rewriting (certain
+/// interactions with macros, for example) and it will fail if any invariants
+/// are violated relating to bound nodes in the match. However, it does not
+/// fail in the case of conflicting edits -- conflict handling is left to
+/// clients. We recommend use of the \c AtomicChange or \c Replacements classes
+/// for assistance in detecting such conflicts.
+EditGenerator editList(llvm::SmallVector<ASTEdit, 1> Edits);
+// Convenience form of `editList` for a single edit.
+EditGenerator edit(ASTEdit);
+
/// Format of the path in an include directive -- angle brackets or quotes.
enum class IncludeFormat {
Quoted,
@@ -106,7 +132,7 @@ enum class IncludeFormat {
struct RewriteRule {
struct Case {
ast_matchers::internal::DynTypedMatcher Matcher;
- SmallVector<ASTEdit, 1> Edits;
+ EditGenerator Edits;
TextGenerator Explanation;
// Include paths to add to the file affected by this case. These are
// bundled with the `Case`, rather than the `RewriteRule`, because each case
@@ -123,16 +149,22 @@ struct RewriteRule {
/// Convenience function for constructing a simple \c RewriteRule.
RewriteRule makeRule(ast_matchers::internal::DynTypedMatcher M,
- SmallVector<ASTEdit, 1> Edits,
- TextGenerator Explanation = nullptr);
+ EditGenerator Edits, TextGenerator Explanation = nullptr);
+
+/// Convenience function for constructing a \c RewriteRule from multiple
+/// `ASTEdit`s.
+inline RewriteRule makeRule(ast_matchers::internal::DynTypedMatcher M,
+ llvm::SmallVector<ASTEdit, 1> Edits,
+ TextGenerator Explanation = nullptr) {
+ return makeRule(std::move(M), editList(std::move(Edits)),
+ std::move(Explanation));
+}
/// Convenience overload of \c makeRule for common case of only one edit.
inline RewriteRule makeRule(ast_matchers::internal::DynTypedMatcher M,
ASTEdit Edit,
TextGenerator Explanation = nullptr) {
- SmallVector<ASTEdit, 1> Edits;
- Edits.emplace_back(std::move(Edit));
- return makeRule(std::move(M), std::move(Edits), std::move(Explanation));
+ return makeRule(std::move(M), edit(std::move(Edit)), std::move(Explanation));
}
/// For every case in Rule, adds an include directive for the given header. The
@@ -260,28 +292,6 @@ getRuleMatchLoc(const ast_matchers::MatchFinder::MatchResult &Result);
const RewriteRule::Case &
findSelectedCase(const ast_matchers::MatchFinder::MatchResult &Result,
const RewriteRule &Rule);
-
-/// A source "transformation," represented by a character range in the source to
-/// be replaced and a corresponding replacement string.
-struct Transformation {
- CharSourceRange Range;
- std::string Replacement;
-};
-
-/// Attempts to translate `Edits`, which are in terms of AST nodes bound in the
-/// match `Result`, into Transformations, which are in terms of the source code
-/// text.
-///
-/// Returns an empty vector if any of the edits apply to portions of the source
-/// that are ineligible for rewriting (certain interactions with macros, for
-/// example). Fails if any invariants are violated relating to bound nodes in
-/// the match. However, it does not fail in the case of conflicting edits --
-/// conflict handling is left to clients. We recommend use of the \c
-/// AtomicChange or \c Replacements classes for assistance in detecting such
-/// conflicts.
-Expected<SmallVector<Transformation, 1>>
-translateEdits(const ast_matchers::MatchFinder::MatchResult &Result,
- llvm::ArrayRef<ASTEdit> Edits);
} // namespace detail
} // namespace transformer
diff --git a/clang/lib/Tooling/Transformer/RewriteRule.cpp b/clang/lib/Tooling/Transformer/RewriteRule.cpp
index 599f20b18eb6..968f7fa6cd32 100644
--- a/clang/lib/Tooling/Transformer/RewriteRule.cpp
+++ b/clang/lib/Tooling/Transformer/RewriteRule.cpp
@@ -28,12 +28,11 @@ using ast_matchers::internal::DynTypedMatcher;
using MatchResult = MatchFinder::MatchResult;
-Expected<SmallVector<transformer::detail::Transformation, 1>>
-transformer::detail::translateEdits(const MatchResult &Result,
- llvm::ArrayRef<ASTEdit> Edits) {
- SmallVector<transformer::detail::Transformation, 1> Transformations;
- for (const auto &Edit : Edits) {
- Expected<CharSourceRange> Range = Edit.TargetRange(Result);
+static Expected<SmallVector<transformer::Edit, 1>>
+translateEdits(const MatchResult &Result, ArrayRef<ASTEdit> ASTEdits) {
+ SmallVector<transformer::Edit, 1> Edits;
+ for (const auto &E : ASTEdits) {
+ Expected<CharSourceRange> Range = E.TargetRange(Result);
if (!Range)
return Range.takeError();
llvm::Optional<CharSourceRange> EditRange =
@@ -41,21 +40,33 @@ transformer::detail::translateEdits(const MatchResult &Result,
// FIXME: let user specify whether to treat this case as an error or ignore
// it as is currently done.
if (!EditRange)
- return SmallVector<Transformation, 0>();
- auto Replacement = Edit.Replacement->eval(Result);
+ return SmallVector<Edit, 0>();
+ auto Replacement = E.Replacement->eval(Result);
if (!Replacement)
return Replacement.takeError();
- transformer::detail::Transformation T;
+ transformer::Edit T;
T.Range = *EditRange;
T.Replacement = std::move(*Replacement);
- Transformations.push_back(std::move(T));
+ Edits.push_back(std::move(T));
}
- return Transformations;
+ return Edits;
}
-ASTEdit transformer::changeTo(RangeSelector S, TextGenerator Replacement) {
+EditGenerator transformer::editList(SmallVector<ASTEdit, 1> Edits) {
+ return [Edits = std::move(Edits)](const MatchResult &Result) {
+ return translateEdits(Result, Edits);
+ };
+}
+
+EditGenerator transformer::edit(ASTEdit Edit) {
+ return [Edit = std::move(Edit)](const MatchResult &Result) {
+ return translateEdits(Result, {Edit});
+ };
+}
+
+ASTEdit transformer::changeTo(RangeSelector Target, TextGenerator Replacement) {
ASTEdit E;
- E.TargetRange = std::move(S);
+ E.TargetRange = std::move(Target);
E.Replacement = std::move(Replacement);
return E;
}
@@ -82,8 +93,9 @@ ASTEdit transformer::remove(RangeSelector S) {
return change(std::move(S), std::make_shared<SimpleTextGenerator>(""));
}
-RewriteRule transformer::makeRule(DynTypedMatcher M, SmallVector<ASTEdit, 1> Edits,
- TextGenerator Explanation) {
+RewriteRule transformer::makeRule(ast_matchers::internal::DynTypedMatcher M,
+ EditGenerator Edits,
+ TextGenerator Explanation) {
return RewriteRule{{RewriteRule::Case{
std::move(M), std::move(Edits), std::move(Explanation), {}}}};
}
diff --git a/clang/lib/Tooling/Transformer/Transformer.cpp b/clang/lib/Tooling/Transformer/Transformer.cpp
index 71f0646f4c0e..93c2c0912d21 100644
--- a/clang/lib/Tooling/Transformer/Transformer.cpp
+++ b/clang/lib/Tooling/Transformer/Transformer.cpp
@@ -31,7 +31,7 @@ void Transformer::run(const MatchFinder::MatchResult &Result) {
transformer::RewriteRule::Case Case =
transformer::detail::findSelectedCase(Result, Rule);
- auto Transformations = transformer::detail::translateEdits(Result, Case.Edits);
+ auto Transformations = Case.Edits(Result);
if (!Transformations) {
Consumer(Transformations.takeError());
return;
More information about the cfe-commits
mailing list