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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel marked 5 inline comments as done.
ymandel added inline comments.


================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:76
+// `ASTEdit` should be built using the corresponding builder class,
+// `ASTEditBuilder`. For example,
+// \code
----------------
ilya-biryukov wrote:
> Comment and the code example still mention ASTBuilder.
Thanks. I noticed I was also using an older version that integrated with Stencil. I've updated to make that explicit. I expect Stencil to land really soon (and can wait to commit until then), but I can remove the reference if you prefer.  Without  Stencils, though, the examples are pretty lame.

I also noticed other spurious mentions of "builder" below and deleted them as well.


================
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);
+
----------------
ilya-biryukov wrote:
> 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)");
Good idea. changed as you suggested.


================
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.
----------------
ilya-biryukov wrote:
> Are `addition/removal of headers ` something that is going to land in the future?
> 
> Maybe leave out this comment until it actually lands?
Good catch


================
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;
----------------
ilya-biryukov wrote:
> Could we add a test for this case?
> (Sorry for spam if there's one already and I missed it)
Nope -- I hadn't tested this yet. Thanks!


================
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)) {
----------------
ilya-biryukov wrote:
> 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.
Done. I personally always prefer braces on for loops, so always happy to add them. ;)


================
Comment at: clang/unittests/Tooling/TransformerTest.cpp:355
 
 //
 // Negative tests (where we expect no transformation to occur).
----------------
ilya-biryukov wrote:
> Could we add a test with intersecting ranges?
> To have a regression test in case we'll change behavior in this corner case later.
Added two tests: one for conflict in single match, one for conflict between matches.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60408/new/

https://reviews.llvm.org/D60408