[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.

```
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





More information about the cfe-commits mailing list