[PATCH] D60408: [LibTooling] Extend Transformer to support multiple simultaneous changes.

Ilya Biryukov via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Apr 16 04:44:18 PDT 2019

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.


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.

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list