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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
