ilya-biryukov added a comment.
Thanks for the update, looks good! Just a few nits from my side.
> The only part I don't love (here and elsewhere) is the duplicate overloads
> one each for std::string and TextGenerator.
Totally agree, that's not ideal. Would be nice to find a way to workaround this.
================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:76
+// `ASTEdit` should be built using the corresponding builder class,
+// `ASTEditBuilder`. For example,
+// \code
----------------
Comment and the code example still mention ASTBuilder.
================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:132
+/// Variant of \c change that selects the node of the entire match.
+template <typename T> ASTEdit changeMatched(TextGenerator Replacement);
+
----------------
Any reason to use a different name? Having an overload for `change` without
`Target` does not look confusing.
```
change<clang::Expr>("replacement(1,2,3)");
vs
changeMatched<clang::Expr>("replacement(1,2,3)");
================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:147
//
-// * Target: the source code impacted by the rule. This identifies an AST node,
-// or part thereof (\c TargetPart), whose source range indicates the extent
of
-// the replacement applied by the replacement term. By default, the extent
is
-// the node matched by the pattern term (\c NodePart::Node). Target's are
-// typed (\c TargetKind), which guides the determination of the node extent
-// and might, in the future, statically constrain the set of eligible
-// NodeParts for a given node.
-//
-// * Replacement: a function that produces a replacement string for the target,
-// based on the match result.
+// * Edits: a set of Edits to the source code, including addition/removal of
+// headers.
----------------
Are `addition/removal of headers ` something that is going to land in the
future?
Maybe leave out this comment until it actually lands?
================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:185
void Transformer::run(const MatchResult &Result) {
- auto ChangeOrErr = applyRewriteRule(Rule, Result);
- if (auto Err = ChangeOrErr.takeError()) {
- llvm::errs() << "Rewrite failed: " << llvm::toString(std::move(Err))
+ if (Result.Context->getDiagnostics().hasErrorOccurred())
+ return;
----------------
Could we add a test for this case?
(Sorry for spam if there's one already and I missed it)
================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:213
+ AtomicChange AC(*Result.SourceManager, RootLoc);
+ for (const auto &T : Transformations)
+ if (auto Err = AC.replace(*Result.SourceManager, T.Range, T.Replacement)) {
----------------
NIT: maybe add braces?
I believe they're not necessary in LLVM Style, but IMO add readability in case
of nested statements with their own braces, like `if` in this code.
================
Comment at: clang/unittests/Tooling/TransformerTest.cpp:355
//
// Negative tests (where we expect no transformation to occur).
----------------
Could we add a test with intersecting ranges?
To have a regression test in case we'll change behavior in this corner case
later.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60408/new/
https://reviews.llvm.org/D60408
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
