[PATCH] D61386: [clang-tidy] Add support writing a check as a Transformer rewrite rule.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp:46
+
+  StringRef Message = "no explanation";
+  if (Case.Explanation) {

ilya-biryukov wrote:
> ymandel wrote:
> > ilya-biryukov wrote:
> > > ymandel wrote:
> > > > ilya-biryukov wrote:
> > > > > The users will see this for every case without explanation, right?
> > > > > I'd expect the rules without explanation to be somewhat common, maybe 
> > > > > don't show any message at all in that case?
> > > > There's no option to call `diag()` without a message.  We could pass an 
> > > > empty string , but that may be confusing given the way the message is 
> > > > concatenated here:
> > > > https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp#L204
> > > > 
> > > > So, no matter what, there will be some message to go w/ the diagnostic. 
> > > > I figure that being explicit about the lack of explanation is better 
> > > > than an empty string, but don't feel strongly about this.
> > > Ah, so all the options are bad. I can see why you had this design in 
> > > transformers in the first place.
> > > I wonder if we should check the explanations are always set for rewrite 
> > > rules inside the clang-tidy transformation?
> > > Ah, so all the options are bad. I can see why you had this design in 
> > > transformers in the first place.
> > Heh. indeed.
> > 
> > > I wonder if we should check the explanations are always set for rewrite 
> > > rules inside the clang-tidy transformation?> Quoted Text
> > 
> > I would have thought so, but AFAIK, most folks who write one-off 
> > transformations use clang-tidy, rather than writing a standalone tool. They 
> > just ignore the diagnostics, i gather.  Transformer may shift that somewhat 
> > if we improve the experience of writing a (throwaway) standalone tool, but 
> > for the time being I think we can't assume that.
> > 
> We should focus on minimizing maintenance cost for long-term fixes rather 
> than one-off transformations. The cost of passing an empty string to a 
> required explanation field for one-off transformations is rather small and 
> falls into the hands of a person writing the check, the cost of constantly 
> finding and fixing the long-lived upstream checks that don't have explanation 
> (leading to bad user experience) is high and will probably fall into the 
> hands of `clang-tidy` maintainers in addition to people writing the checks.
> 
> FWIW, having a new API of top of plain transformers should be better than 
> `clang-tidy` for those writing one-off transformations in the long run. I 
> don't think `clang-tidy` was ever designed to cover to cover those use-cases, 
> even though today it seems to be the fastest way to do those.
Agreed on all points. I'll send a followup that enforces that constraint.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61386



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61386: [clang-tidy] Add support writing a check as a Transformer rewrite rule.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp:46
+
+  StringRef Message = "no explanation";
+  if (Case.Explanation) {

ymandel wrote:
> ilya-biryukov wrote:
> > ymandel wrote:
> > > ilya-biryukov wrote:
> > > > The users will see this for every case without explanation, right?
> > > > I'd expect the rules without explanation to be somewhat common, maybe 
> > > > don't show any message at all in that case?
> > > There's no option to call `diag()` without a message.  We could pass an 
> > > empty string , but that may be confusing given the way the message is 
> > > concatenated here:
> > > https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp#L204
> > > 
> > > So, no matter what, there will be some message to go w/ the diagnostic. I 
> > > figure that being explicit about the lack of explanation is better than 
> > > an empty string, but don't feel strongly about this.
> > Ah, so all the options are bad. I can see why you had this design in 
> > transformers in the first place.
> > I wonder if we should check the explanations are always set for rewrite 
> > rules inside the clang-tidy transformation?
> > Ah, so all the options are bad. I can see why you had this design in 
> > transformers in the first place.
> Heh. indeed.
> 
> > I wonder if we should check the explanations are always set for rewrite 
> > rules inside the clang-tidy transformation?> Quoted Text
> 
> I would have thought so, but AFAIK, most folks who write one-off 
> transformations use clang-tidy, rather than writing a standalone tool. They 
> just ignore the diagnostics, i gather.  Transformer may shift that somewhat 
> if we improve the experience of writing a (throwaway) standalone tool, but 
> for the time being I think we can't assume that.
> 
We should focus on minimizing maintenance cost for long-term fixes rather than 
one-off transformations. The cost of passing an empty string to a required 
explanation field for one-off transformations is rather small and falls into 
the hands of a person writing the check, the cost of constantly finding and 
fixing the long-lived upstream checks that don't have explanation (leading to 
bad user experience) is high and will probably fall into the hands of 
`clang-tidy` maintainers in addition to people writing the checks.

FWIW, having a new API of top of plain transformers should be better than 
`clang-tidy` for those writing one-off transformations in the long run. I don't 
think `clang-tidy` was ever designed to cover to cover those use-cases, even 
though today it seems to be the fastest way to do those.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61386



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61386: [clang-tidy] Add support writing a check as a Transformer rewrite rule.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL361418: [clang-tidy] Add support for writing a check as a 
Transformer rewrite rule. (authored by ymandel, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D61386?vs=200802&id=200804#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D61386

Files:
  clang-tools-extra/trunk/clang-tidy/utils/CMakeLists.txt
  clang-tools-extra/trunk/clang-tidy/utils/TransformerClangTidyCheck.cpp
  clang-tools-extra/trunk/clang-tidy/utils/TransformerClangTidyCheck.h
  clang-tools-extra/trunk/unittests/clang-tidy/CMakeLists.txt
  clang-tools-extra/trunk/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp

Index: clang-tools-extra/trunk/clang-tidy/utils/TransformerClangTidyCheck.h
===
--- clang-tools-extra/trunk/clang-tidy/utils/TransformerClangTidyCheck.h
+++ clang-tools-extra/trunk/clang-tidy/utils/TransformerClangTidyCheck.h
@@ -0,0 +1,49 @@
+//===-- TransformerClangTidyCheck.h - clang-tidy --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_TRANSFORMER_CLANG_TIDY_CHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_TRANSFORMER_CLANG_TIDY_CHECK_H
+
+#include "../ClangTidy.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Tooling/Refactoring/Transformer.h"
+#include 
+#include 
+
+namespace clang {
+namespace tidy {
+namespace utils {
+
+// A base class for defining a ClangTidy check based on a `RewriteRule`.
+//
+// For example, given a rule `MyCheckAsRewriteRule`, one can define a tidy check
+// as follows:
+//
+// class MyCheck : public TransformerClangTidyCheck {
+//  public:
+//   MyCheck(StringRef Name, ClangTidyContext *Context)
+//   : TransformerClangTidyCheck(MyCheckAsRewriteRule, Name, Context) {}
+// };
+class TransformerClangTidyCheck : public ClangTidyCheck {
+public:
+  TransformerClangTidyCheck(tooling::RewriteRule R, StringRef Name,
+ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context), Rule(std::move(R)) {}
+
+  void registerMatchers(ast_matchers::MatchFinder *Finder) final;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) final;
+
+private:
+  tooling::RewriteRule Rule;
+};
+
+} // namespace utils
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_TRANSFORMER_CLANG_TIDY_CHECK_H
Index: clang-tools-extra/trunk/clang-tidy/utils/TransformerClangTidyCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/utils/TransformerClangTidyCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/utils/TransformerClangTidyCheck.cpp
@@ -0,0 +1,63 @@
+//===-- TransformerClangTidyCheck.cpp - clang-tidy ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "TransformerClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace utils {
+using tooling::RewriteRule;
+
+void TransformerClangTidyCheck::registerMatchers(
+ast_matchers::MatchFinder *Finder) {
+  Finder->addDynamicMatcher(tooling::detail::buildMatcher(Rule), this);
+}
+
+void TransformerClangTidyCheck::check(
+const ast_matchers::MatchFinder::MatchResult &Result) {
+  if (Result.Context->getDiagnostics().hasErrorOccurred())
+return;
+
+  // Verify the existence and validity of the AST node that roots this rule.
+  const ast_matchers::BoundNodes::IDToNodeMap &NodesMap = Result.Nodes.getMap();
+  auto Root = NodesMap.find(RewriteRule::RootID);
+  assert(Root != NodesMap.end() && "Transformation failed: missing root node.");
+  SourceLocation RootLoc = Result.SourceManager->getExpansionLoc(
+  Root->second.getSourceRange().getBegin());
+  assert(RootLoc.isValid() && "Invalid location for Root node of match.");
+
+  RewriteRule::Case Case = tooling::detail::findSelectedCase(Result, Rule);
+  Expected> Transformations =
+  tooling::detail::translateEdits(Result, Case.Edits);
+  if (!Transformations) {
+llvm::errs() << "Rewrite failed: "
+ << llvm::toString(Transformations.takeError()) << "\n";
+return;
+  }
+
+  // No rewrite applied, but no error encountered either.
+  if (Transformations->empty())
+return;
+
+  StringRef Message = "no explanation";
+  if (Case.Explanation) {
+if (Expected E = Case.Explanation(Re

[PATCH] D61386: [clang-tidy] Add support writing a check as a Transformer rewrite rule.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 200802.
ymandel removed a subscriber: dexonsmith.
ymandel added a comment.

Replace previous diff which had the wrong base.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61386

Files:
  clang-tools-extra/clang-tidy/utils/CMakeLists.txt
  clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
  clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h
  clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
  clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp

Index: clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
===
--- /dev/null
+++ clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
@@ -0,0 +1,68 @@
+//=== TransformerClangTidyCheckTest.cpp - clang-tidy --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "../clang-tidy/utils/TransformerClangTidyCheck.h"
+#include "ClangTidyTest.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Refactoring/RangeSelector.h"
+#include "clang/Tooling/Refactoring/Stencil.h"
+#include "clang/Tooling/Refactoring/Transformer.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tidy {
+namespace utils {
+namespace {
+using tooling::RewriteRule;
+
+// Invert the code of an if-statement, while maintaining its semantics.
+RewriteRule invertIf() {
+  using namespace ::clang::ast_matchers;
+  using tooling::change;
+  using tooling::node;
+  using tooling::statement;
+  using tooling::stencil::cat;
+
+  StringRef C = "C", T = "T", E = "E";
+  return tooling::makeRule(ifStmt(hasCondition(expr().bind(C)),
+  hasThen(stmt().bind(T)),
+  hasElse(stmt().bind(E))),
+   change(statement(RewriteRule::RootID),
+  cat("if(!(", node(C), ")) ", statement(E),
+  " else ", statement(T;
+}
+
+class IfInverterCheck : public TransformerClangTidyCheck {
+public:
+  IfInverterCheck(StringRef Name, ClangTidyContext *Context)
+  : TransformerClangTidyCheck(invertIf(), Name, Context) {}
+};
+
+// Basic test of using a rewrite rule as a ClangTidy.
+TEST(TransformerClangTidyCheckTest, Basic) {
+  const std::string Input = R"cc(
+void log(const char* msg);
+void foo() {
+  if (10 > 1.0)
+log("oh no!");
+  else
+log("ok");
+}
+  )cc";
+  const std::string Expected = R"(
+void log(const char* msg);
+void foo() {
+  if(!(10 > 1.0)) log("ok"); else log("oh no!");
+}
+  )";
+  EXPECT_EQ(Expected, test::runCheckOnCode(Input));
+}
+} // namespace
+} // namespace utils
+} // namespace tidy
+} // namespace clang
Index: clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
===
--- clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
+++ clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
@@ -17,6 +17,7 @@
   OverlappingReplacementsTest.cpp
   UsingInserterTest.cpp
   ReadabilityModuleTest.cpp
+  TransformerClangTidyCheckTest.cpp
   )
 
 target_link_libraries(ClangTidyTests
@@ -36,4 +37,5 @@
   clangTidyUtils
   clangTooling
   clangToolingCore
+  clangToolingRefactor
   )
Index: clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h
@@ -0,0 +1,49 @@
+//===-- TransformerClangTidyCheck.h - clang-tidy --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_TRANSFORMER_CLANG_TIDY_CHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_TRANSFORMER_CLANG_TIDY_CHECK_H
+
+#include "../ClangTidy.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Tooling/Refactoring/Transformer.h"
+#include 
+#include 
+
+namespace clang {
+namespace tidy {
+namespace utils {
+
+// A base class for defining a ClangTidy check based on a `RewriteRule`.
+//
+// For example, given a rule `MyCheckAsRewriteRule`, one can define a tidy check
+// as follows:
+//
+// class MyCheck : public TransformerClangTidyCheck {
+//  public:
+//   MyCheck(StringRef Name, ClangTidyContext *Context)
+//   : TransformerClangTidyCheck(MyCheckA

[PATCH] D61386: [clang-tidy] Add support writing a check as a Transformer rewrite rule.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp:46
+
+  StringRef Message = "no explanation";
+  if (Case.Explanation) {

ilya-biryukov wrote:
> ymandel wrote:
> > ilya-biryukov wrote:
> > > The users will see this for every case without explanation, right?
> > > I'd expect the rules without explanation to be somewhat common, maybe 
> > > don't show any message at all in that case?
> > There's no option to call `diag()` without a message.  We could pass an 
> > empty string , but that may be confusing given the way the message is 
> > concatenated here:
> > https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp#L204
> > 
> > So, no matter what, there will be some message to go w/ the diagnostic. I 
> > figure that being explicit about the lack of explanation is better than an 
> > empty string, but don't feel strongly about this.
> Ah, so all the options are bad. I can see why you had this design in 
> transformers in the first place.
> I wonder if we should check the explanations are always set for rewrite rules 
> inside the clang-tidy transformation?
> Ah, so all the options are bad. I can see why you had this design in 
> transformers in the first place.
Heh. indeed.

> I wonder if we should check the explanations are always set for rewrite rules 
> inside the clang-tidy transformation?> Quoted Text

I would have thought so, but AFAIK, most folks who write one-off 
transformations use clang-tidy, rather than writing a standalone tool. They 
just ignore the diagnostics, i gather.  Transformer may shift that somewhat if 
we improve the experience of writing a (throwaway) standalone tool, but for the 
time being I think we can't assume that.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61386



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61386: [clang-tidy] Add support writing a check as a Transformer rewrite rule.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked an inline comment as done.
ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp:46
+
+  StringRef Message = "no explanation";
+  if (Case.Explanation) {

ymandel wrote:
> ilya-biryukov wrote:
> > The users will see this for every case without explanation, right?
> > I'd expect the rules without explanation to be somewhat common, maybe don't 
> > show any message at all in that case?
> There's no option to call `diag()` without a message.  We could pass an empty 
> string , but that may be confusing given the way the message is concatenated 
> here:
> https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp#L204
> 
> So, no matter what, there will be some message to go w/ the diagnostic. I 
> figure that being explicit about the lack of explanation is better than an 
> empty string, but don't feel strongly about this.
Ah, so all the options are bad. I can see why you had this design in 
transformers in the first place.
I wonder if we should check the explanations are always set for rewrite rules 
inside the clang-tidy transformation?



Comment at: clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h:1
+//===-- TransformerClangTidyCheck.h - clang-tidy 
--===//
+//

ymandel wrote:
> ilya-biryukov wrote:
> > NIT: maybe use `TransformerCheck` for brevity? I'm not a `clang-tidy` 
> > maintainer, though, so not sure whether that aligns with the rest of the 
> > code.
> It was TransformerTidy and Dmitri suggested I be explicit. I think 
> TransformerCheck is better than TransformerTidy, but I also see the argument 
> in spelling it out.  WDYT?
Let's stick with Dmitri's suggestion. He has much more experience with LLVM 
than I do :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61386



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61386: [clang-tidy] Add support writing a check as a Transformer rewrite rule.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp:46
+
+  StringRef Message = "no explanation";
+  if (Case.Explanation) {

ilya-biryukov wrote:
> The users will see this for every case without explanation, right?
> I'd expect the rules without explanation to be somewhat common, maybe don't 
> show any message at all in that case?
There's no option to call `diag()` without a message.  We could pass an empty 
string , but that may be confusing given the way the message is concatenated 
here:
https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp#L204

So, no matter what, there will be some message to go w/ the diagnostic. I 
figure that being explicit about the lack of explanation is better than an 
empty string, but don't feel strongly about this.



Comment at: clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h:1
+//===-- TransformerClangTidyCheck.h - clang-tidy 
--===//
+//

ilya-biryukov wrote:
> NIT: maybe use `TransformerCheck` for brevity? I'm not a `clang-tidy` 
> maintainer, though, so not sure whether that aligns with the rest of the code.
It was TransformerTidy and Dmitri suggested I be explicit. I think 
TransformerCheck is better than TransformerTidy, but I also see the argument in 
spelling it out.  WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61386



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61386: [clang-tidy] Add support writing a check as a Transformer rewrite rule.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp:46
+
+  StringRef Message = "no explanation";
+  if (Case.Explanation) {

The users will see this for every case without explanation, right?
I'd expect the rules without explanation to be somewhat common, maybe don't 
show any message at all in that case?



Comment at: clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h:1
+//===-- TransformerClangTidyCheck.h - clang-tidy 
--===//
+//

NIT: maybe use `TransformerCheck` for brevity? I'm not a `clang-tidy` 
maintainer, though, so not sure whether that aligns with the rest of the code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61386



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61386: [clang-tidy] Add support writing a check as a Transformer rewrite rule.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

In D61386#1510690 , @MyDeveloperDay 
wrote:

> Just a passing comment, Is this:
>
> a) a different type of check that would be used for a different type of check 
> than previous clang-tidy checks
>  b) or is this a replacement on the previous style?


This is a replacement for a certain class of checks. Mostly for simple tests 
that are primarily pattern + reconstruction, without complex logic (at least 
for now).  The long-term goal is to provide a concise alternative for the large 
majority of checks.

> Is there any documentation about the benefits of this method vs the old 
> method? or of why we would potentially use one and not the other?

There's a doc describing the framework here
https://docs.google.com/document/d/1ppw0RhjwsrbBcHYhI85pe6ISDbA6r5d00ot3N8cQWeQ/edit?usp=sharing
and some corresponding discussion on the cfe-dev mailing list (and in the doc 
comments).

> Is there a change needed to add_new_check.py to have it generate this new 
> style? (its ok if the answer is yes in a future revision)
> 
> Do you foresee people using this mechanism going forward?

Not yet, but once it is more mature, yes. I would hope that it will be the more 
common approach. At that point, we can adjust the scripts accordingly.

> I do like the brevity of it and I do like the GTEST unit tests, (I struggled 
> more with the lit tests than anything else, mainly for python on cygwin 
> reasons)
> 
> It looks good, even if it looks like I need to go learn how tooling::stencil 
> works... a little bit of documentation might not go a miss on how to get 
> started with it...

Thanks.  Yes, it very much requires that you familiarize yourself with Stencil 
and RewriteRule (Tooling/Refactoring/Stencil.h and Transformer.h).  I'm not 
sure what more to add that is specific to clang tidy. The intent is that, if 
your check can be expressed as a `RewriteRule` then there's really nothing you 
need to know about clang-tidy beyond what's in the header file -- you just drop 
the rule into TransformerClangTidyCheck.  If you have questions after you see 
the Tooling documentation, please let me know and I'm happy to expand the 
comments here.  Both Stencil and Transformer are quite and still under active 
development. Once they've stabilized a bit, I also plan to send out a more 
general announcement about this to the cfe-dev list.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61386



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61386: [clang-tidy] Add support writing a check as a Transformer rewrite rule.

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

Just a passing comment, Is this:

a) a different type of check that would be used for a different type of check 
than previous clang-tidy checks
b) or is this a replacement on the previous style?

Is there any documentation about the benefits of this method vs the old method? 
or of why we would potentially use one and not the other?

Is there a change needed to add_new_check.py to have it generate this new 
style? (its ok if the answer is yes in a future revision)

Do you foresee people using this mechanism going foward?

I do like the brevity of it and I do like the GTEST unit tests, (I struggled 
more with the lit tests than anything else, mainly for python on cygwin reasons)

It looks good, even if it looks like I need to go learn how tooling::stencil 
works... a little bit of documentation might not go a miss on how to get 
started with it...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61386



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61386: [clang-tidy] Add support writing a check as a Transformer rewrite rule.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel marked 4 inline comments as done.
ymandel added a comment.

Thanks for the review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61386



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61386: [clang-tidy] Add support writing a check as a Transformer rewrite rule.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 200536.
ymandel added a comment.

Renamed TransformerTidy to TransformerClangTidyCheck; classes and files.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61386

Files:
  clang-tools-extra/clang-tidy/utils/CMakeLists.txt
  clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
  clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h
  clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
  clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp

Index: clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
===
--- /dev/null
+++ clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
@@ -0,0 +1,63 @@
+//=== TransformerClangTidyCheckTest.cpp - clang-tidy --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "../clang-tidy/utils/TransformerClangTidyCheck.h"
+#include "ClangTidyTest.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Refactoring/Stencil.h"
+#include "clang/Tooling/Refactoring/Transformer.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tidy {
+namespace utils {
+namespace {
+// Invert the code of an if-statement, while maintaining its semantics.
+tooling::RewriteRule invertIf() {
+  using namespace ::clang::ast_matchers;
+  using tooling::change;
+  using tooling::stencil::cat;
+  using tooling::stencil::node;
+  using tooling::stencil::sNode;
+
+  StringRef C = "C", T = "T", E = "E";
+  return tooling::makeRule(
+  ifStmt(hasCondition(expr().bind(C)), hasThen(stmt().bind(T)),
+ hasElse(stmt().bind(E))),
+  change(cat("if(!(", node(C), ")) ", sNode(E), " else ", sNode(T;
+}
+
+class IfInverterCheck : public TransformerClangTidyCheck {
+public:
+  IfInverterCheck(StringRef Name, ClangTidyContext *Context)
+  : TransformerClangTidyCheck(invertIf(), Name, Context) {}
+};
+
+// Basic test of using a rewrite rule as a ClangTidy.
+TEST(TransformerClangTidyCheckTest, Basic) {
+  const std::string Input = R"cc(
+void log(const char* msg);
+void foo() {
+  if (10 > 1.0)
+log("oh no!");
+  else
+log("ok");
+}
+  )cc";
+  const std::string Expected = R"(
+void log(const char* msg);
+void foo() {
+  if(!(10 > 1.0)) log("ok"); else log("oh no!");
+}
+  )";
+  EXPECT_EQ(Expected, test::runCheckOnCode(Input));
+}
+} // namespace
+} // namespace utils
+} // namespace tidy
+} // namespace clang
Index: clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
===
--- clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
+++ clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
@@ -17,6 +17,7 @@
   OverlappingReplacementsTest.cpp
   UsingInserterTest.cpp
   ReadabilityModuleTest.cpp
+  TransformerClangTidyCheckTest.cpp
   )
 
 target_link_libraries(ClangTidyTests
@@ -36,4 +37,5 @@
   clangTidyUtils
   clangTooling
   clangToolingCore
+  clangToolingRefactor
   )
Index: clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h
@@ -0,0 +1,49 @@
+//===-- TransformerClangTidyCheck.h - clang-tidy --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_TRANSFORMER_CLANG_TIDY_CHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_TRANSFORMER_CLANG_TIDY_CHECK_H
+
+#include "../ClangTidy.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Tooling/Refactoring/Transformer.h"
+#include 
+#include 
+
+namespace clang {
+namespace tidy {
+namespace utils {
+
+// A base class for defining a ClangTidy check based on a `RewriteRule`.
+//
+// For example, given a rule `MyCheckAsRewriteRule`, one can define a tidy check
+// as follows:
+//
+// class MyCheck : public TransformerClangTidyCheck {
+//  public:
+//   MyCheck(StringRef Name, ClangTidyContext *Context)
+//   : TransformerClangTidyCheck(MyCheckAsRewriteRule, Name, Context) {}
+// };
+class TransformerClangTidyCheck : public ClangTidyCheck {
+public:
+  TransformerClangTidyCheck(tooling::RewriteRule R, StringRef Name,
+ClangTidyContext *Context)
+  : ClangTidyCheck(Name, 

[PATCH] D61386: [clang-tidy] Add support writing a check as a Transformer rewrite rule.

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/TransformerTidy.h:32
+// };
+class TransformerTidy : public ClangTidyCheck {
+public:

ymandel wrote:
> gribozavr wrote:
> > I'd prefer a name like "TransformerClangTidyCheck", it will only appear in 
> > a few places, but is much more clear.
> will do, but should I also rename the files correspondingly?
Yes please.



Comment at: clang-tools-extra/clang-tidy/utils/TransformerTidy.h:38
+
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;

ymandel wrote:
> gribozavr wrote:
> > I'd suggest to add comments telling users to not override these methods in 
> > subclasses.
> sure. would marking them as `final` make sense, then?
Right, that's the perfect tool for the job.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61386



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61386: [clang-tidy] Add support writing a check as a Transformer rewrite rule.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/clang-tidy/utils/TransformerTidy.h:32
+// };
+class TransformerTidy : public ClangTidyCheck {
+public:

gribozavr wrote:
> I'd prefer a name like "TransformerClangTidyCheck", it will only appear in a 
> few places, but is much more clear.
will do, but should I also rename the files correspondingly?



Comment at: clang-tools-extra/clang-tidy/utils/TransformerTidy.h:38
+
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;

gribozavr wrote:
> I'd suggest to add comments telling users to not override these methods in 
> subclasses.
sure. would marking them as `final` make sense, then?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61386



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61386: [clang-tidy] Add support writing a check as a Transformer rewrite rule.

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr accepted this revision.
gribozavr added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tools-extra/clang-tidy/utils/TransformerTidy.h:32
+// };
+class TransformerTidy : public ClangTidyCheck {
+public:

I'd prefer a name like "TransformerClangTidyCheck", it will only appear in a 
few places, but is much more clear.



Comment at: clang-tools-extra/clang-tidy/utils/TransformerTidy.h:38
+
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;

I'd suggest to add comments telling users to not override these methods in 
subclasses.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61386



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61386: [clang-tidy] Add support writing a check as a Transformer rewrite rule.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

Ping.  Is anyone willing to be the reviewer for this revision?

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61386



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61386: [clang-tidy] Add support writing a check as a Transformer rewrite rule.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 200072.
ymandel added a comment.

Updated to incorporate changes to Transformer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61386

Files:
  clang-tools-extra/clang-tidy/utils/CMakeLists.txt
  clang-tools-extra/clang-tidy/utils/TransformerTidy.cpp
  clang-tools-extra/clang-tidy/utils/TransformerTidy.h
  clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
  clang-tools-extra/unittests/clang-tidy/TransformerTidyTest.cpp

Index: clang-tools-extra/unittests/clang-tidy/TransformerTidyTest.cpp
===
--- /dev/null
+++ clang-tools-extra/unittests/clang-tidy/TransformerTidyTest.cpp
@@ -0,0 +1,63 @@
+//=== TransformerTidyTest.cpp - clang-tidy ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "../clang-tidy/utils/TransformerTidy.h"
+#include "ClangTidyTest.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Refactoring/Stencil.h"
+#include "clang/Tooling/Refactoring/Transformer.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tidy {
+namespace utils {
+namespace {
+// Invert the code of an if-statement, while maintaining its semantics.
+tooling::RewriteRule invertIf() {
+  using namespace ::clang::ast_matchers;
+  using tooling::change;
+  using tooling::stencil::cat;
+  using tooling::stencil::node;
+  using tooling::stencil::sNode;
+
+  StringRef C = "C", T = "T", E = "E";
+  return tooling::makeRule(
+  ifStmt(hasCondition(expr().bind(C)), hasThen(stmt().bind(T)),
+ hasElse(stmt().bind(E))),
+  change(cat("if(!(", node(C), ")) ", sNode(E), " else ", sNode(T;
+}
+
+class IfInverterTidy : public TransformerTidy {
+public:
+  IfInverterTidy(StringRef Name, ClangTidyContext *Context)
+  : TransformerTidy(invertIf(), Name, Context) {}
+};
+
+// Basic test of using a rewrite rule as a ClangTidy.
+TEST(TransformerTidyTest, Basic) {
+  const std::string Input = R"cc(
+void log(const char* msg);
+void foo() {
+  if (10 > 1.0)
+log("oh no!");
+  else
+log("ok");
+}
+  )cc";
+  const std::string Expected = R"(
+void log(const char* msg);
+void foo() {
+  if(!(10 > 1.0)) log("ok"); else log("oh no!");
+}
+  )";
+  EXPECT_EQ(Expected, test::runCheckOnCode(Input));
+}
+} // namespace
+} // namespace utils
+} // namespace tidy
+} // namespace clang
Index: clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
===
--- clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
+++ clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
@@ -17,6 +17,7 @@
   OverlappingReplacementsTest.cpp
   UsingInserterTest.cpp
   ReadabilityModuleTest.cpp
+  TransformerTidyTest.cpp
   )
 
 target_link_libraries(ClangTidyTests
@@ -36,4 +37,5 @@
   clangTidyUtils
   clangTooling
   clangToolingCore
+  clangToolingRefactor
   )
Index: clang-tools-extra/clang-tidy/utils/TransformerTidy.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/utils/TransformerTidy.h
@@ -0,0 +1,49 @@
+//===-- TransformerTidy.h - clang-tidy ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_TRANSFORMER_TIDY_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_TRANSFORMER_TIDY_H
+
+#include "../ClangTidy.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Tooling/Refactoring/Transformer.h"
+#include 
+#include 
+
+namespace clang {
+namespace tidy {
+namespace utils {
+
+// A base class for defining a ClangTidy check based on a rewrite rule.
+//
+// For example, given a RewriteRule `MyCheckAsRewriteRule`, one can define your
+// tidy check as follows:
+//
+// class MyTidyCheck : public TransformerTidy {
+//  public:
+//   MyTidyCheck(StringRef Name, ClangTidyContext *Context)
+//   : TransformerTidy(MyCheckAsRewriteRule, Name, Context) {}
+// };
+class TransformerTidy : public ClangTidyCheck {
+public:
+  TransformerTidy(tooling::RewriteRule R, StringRef Name,
+  ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context), Rule(std::move(R)) {}
+
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  tooling:

[PATCH] D61386: [clang-tidy] Add support writing a check as a Transformer rewrite rule.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

@aaron.ballman, would be a good reviewer for this? I'm happy to look at the 
transformer bits, but I'm definitely the wrong one to asses it from the  
`clang-tidy` perspective.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61386



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61386: [clang-tidy] Add support writing a check as a Transformer rewrite rule.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

Gentle ping...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61386



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61386: [clang-tidy] Add support writing a check as a Transformer rewrite rule.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel marked an inline comment as done.
ymandel added a comment.

In D61386#1487735 , @JonasToth wrote:

> I like the new framework!


Great!




Comment at: clang-tools-extra/unittests/clang-tidy/TransformerTidyTest.cpp:38
+  IfInverterTidy(StringRef Name, ClangTidyContext *Context)
+  : TransformerTidy(invertIf(), Name, Context) {}
+};

JonasToth wrote:
> If we allow to pass in a callable, that returns a `RewriteRule`we would have 
> a more flexible framework in clang-tidy.
> Going with this for now is ok in my opinion as the only good usecase that 
> comes to my mind would be statistics.
> 
> But If some transformation needs to remember what has been done before only a 
> `RewriteRule` as state is not sufficient and stateful actions would require 
> global state (MEH!).
> If we allow to pass in a callable, that returns a RewriteRule we would have a 
> more flexible framework in clang-tidy.
> Going with this for now is ok in my opinion as the only good usecase that 
> comes to my mind would be statistics.
I'm fine with something more general, but I don't think I follow the 
particulars you have in mind.  Even w/ a callable, wouldn't that only be 
invoked *once* by clang-tidy, when it creates the class?  What kind of 
statistics do you have in mind?

> 
> But If some transformation needs to remember what has been done before only a 
> RewriteRule as state is not sufficient and stateful actions would require 
> global state (MEH!).

Indeed. The current design does preclude a (non gross) way of collecting state 
over multiple invocations.  Since this is rare, I think that's fine for now.  
But, our plan is to add support for a Translation Unit level API that supports 
things like collecting state, adding and removing header includes, inserting 
using decls and any other TU level operations.









Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61386



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61386: [clang-tidy] Add support writing a check as a Transformer rewrite rule.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 197836.
ymandel marked 2 inline comments as done.
ymandel added a comment.

responded to comments and shortened (unnecessarily long) variable name.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61386

Files:
  clang-tools-extra/clang-tidy/utils/CMakeLists.txt
  clang-tools-extra/clang-tidy/utils/TransformerTidy.cpp
  clang-tools-extra/clang-tidy/utils/TransformerTidy.h
  clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
  clang-tools-extra/unittests/clang-tidy/TransformerTidyTest.cpp

Index: clang-tools-extra/unittests/clang-tidy/TransformerTidyTest.cpp
===
--- /dev/null
+++ clang-tools-extra/unittests/clang-tidy/TransformerTidyTest.cpp
@@ -0,0 +1,63 @@
+//=== TransformerTidyTest.cpp - clang-tidy ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "../clang-tidy/utils/TransformerTidy.h"
+#include "ClangTidyTest.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Refactoring/Stencil.h"
+#include "clang/Tooling/Refactoring/Transformer.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tidy {
+namespace utils {
+namespace {
+// Invert the code of an if-statement, while maintaining its semantics.
+tooling::RewriteRule invertIf() {
+  using namespace ::clang::ast_matchers;
+  using tooling::change;
+  using tooling::stencil::cat;
+  using tooling::stencil::node;
+  using tooling::stencil::sNode;
+
+  StringRef C = "C", "T", E = "E";
+  return tooling::makeRule(
+  ifStmt(hasCondition(expr().bind(C)), hasThen(stmt().bind(T)),
+ hasElse(stmt().bind(E))),
+  change(cat("if(!(", node(C), ")) ", sNode(E), " else ", sNode(T;
+}
+
+class IfInverterTidy : public TransformerTidy {
+public:
+  IfInverterTidy(StringRef Name, ClangTidyContext *Context)
+  : TransformerTidy(invertIf(), Name, Context) {}
+};
+
+// Basic test of using a rewrite rule as a ClangTidy.
+TEST(TransformerTidyTest, Basic) {
+  const std::string Input = R"cc(
+void log(const char* msg);
+void foo() {
+  if (10 > 1.0)
+log("oh no!");
+  else
+log("ok");
+}
+  )cc";
+  const std::string Expected = R"(
+void log(const char* msg);
+void foo() {
+  if(!(10 > 1.0)) log("ok"); else log("oh no!");
+}
+  )";
+  EXPECT_EQ(Expected, test::runCheckOnCode(Input));
+}
+} // namespace
+} // namespace utils
+} // namespace tidy
+} // namespace clang
Index: clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
===
--- clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
+++ clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
@@ -17,6 +17,7 @@
   OverlappingReplacementsTest.cpp
   UsingInserterTest.cpp
   ReadabilityModuleTest.cpp
+  TransformerTidyTest.cpp
   )
 
 target_link_libraries(ClangTidyTests
@@ -36,4 +37,5 @@
   clangTidyUtils
   clangTooling
   clangToolingCore
+  clangToolingRefactor
   )
Index: clang-tools-extra/clang-tidy/utils/TransformerTidy.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/utils/TransformerTidy.h
@@ -0,0 +1,49 @@
+//===-- TransformerTidy.h - clang-tidy ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_TRANSFORMER_TIDY_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_TRANSFORMER_TIDY_H
+
+#include "../ClangTidy.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Tooling/Refactoring/Transformer.h"
+#include 
+#include 
+
+namespace clang {
+namespace tidy {
+namespace utils {
+
+// A base class for defining a ClangTidy check based on a rewrite rule.
+//
+// For example, given a RewriteRule `MyCheckAsRewriteRule`, one can define your
+// tidy check as follows:
+//
+// class MyTidyCheck : public TransformerTidy {
+//  public:
+//   MyTidyCheck(StringRef Name, ClangTidyContext *Context)
+//   : TransformerTidy(MyCheckAsRewriteRule, Name, Context) {}
+// };
+class TransformerTidy : public ClangTidyCheck {
+public:
+  TransformerTidy(tooling::RewriteRule R, StringRef Name,
+  ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context), Rule(std::move(R)) {}
+
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::Matc

[PATCH] D61386: [clang-tidy] Add support writing a check as a Transformer rewrite rule.

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

I like the new framework!




Comment at: clang-tools-extra/clang-tidy/utils/TransformerTidy.cpp:43
+  if (Rule.Explanation) {
+if (Expected Explanation = Rule.Explanation(Result)) {
+  Message = *Explanation;

these if-else are single statement, so the braces can be ellided.



Comment at: clang-tools-extra/unittests/clang-tidy/TransformerTidyTest.cpp:38
+  IfInverterTidy(StringRef Name, ClangTidyContext *Context)
+  : TransformerTidy(invertIf(), Name, Context) {}
+};

If we allow to pass in a callable, that returns a `RewriteRule`we would have a 
more flexible framework in clang-tidy.
Going with this for now is ok in my opinion as the only good usecase that comes 
to my mind would be statistics.

But If some transformation needs to remember what has been done before only a 
`RewriteRule` as state is not sufficient and stateful actions would require 
global state (MEH!).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61386



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61386: [clang-tidy] Add support writing a check as a Transformer rewrite rule.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/clang-tidy/utils/TransformerTidy.cpp:51
+  DiagnosticBuilder Diag = diag(RootLoc, Message);
+  for (const tooling::Transformation &T : *Transformations) {
+Diag << FixItHint::CreateReplacement(T.Range, T.Replacement);

Eugene.Zelenko wrote:
> It's iterator, so auto could be used instead of type.
great, i wasn't sure whether this counted as such.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61386



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61386: [clang-tidy] Add support writing a check as a Transformer rewrite rule.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 197597.
ymandel marked an inline comment as done.
ymandel added a comment.

reinstated an auto


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61386

Files:
  clang-tools-extra/clang-tidy/utils/CMakeLists.txt
  clang-tools-extra/clang-tidy/utils/TransformerTidy.cpp
  clang-tools-extra/clang-tidy/utils/TransformerTidy.h
  clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
  clang-tools-extra/unittests/clang-tidy/TransformerTidyTest.cpp

Index: clang-tools-extra/unittests/clang-tidy/TransformerTidyTest.cpp
===
--- /dev/null
+++ clang-tools-extra/unittests/clang-tidy/TransformerTidyTest.cpp
@@ -0,0 +1,63 @@
+//=== TransformerTidyTest.cpp - clang-tidy ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "../clang-tidy/utils/TransformerTidy.h"
+#include "ClangTidyTest.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Refactoring/Stencil.h"
+#include "clang/Tooling/Refactoring/Transformer.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tidy {
+namespace utils {
+namespace {
+// Invert the code of an if-statement, while maintaining its semantics.
+tooling::RewriteRule invertIf() {
+  using namespace ::clang::ast_matchers;
+  using tooling::change;
+  using tooling::stencil::cat;
+  using tooling::stencil::node;
+  using tooling::stencil::sNode;
+
+  StringRef C = "C", "T", E = "E";
+  return tooling::makeRule(
+  ifStmt(hasCondition(expr().bind(C)), hasThen(stmt().bind(T)),
+ hasElse(stmt().bind(E))),
+  change(cat("if(!(", node(C), ")) ", sNode(E), " else ", sNode(T;
+}
+
+class IfInverterTidy : public TransformerTidy {
+public:
+  IfInverterTidy(StringRef Name, ClangTidyContext *Context)
+  : TransformerTidy(invertIf(), Name, Context) {}
+};
+
+// Basic test of using a rewrite rule as a ClangTidy.
+TEST(TransformerTidyTest, Basic) {
+  const std::string Input = R"cc(
+void log(const char* msg);
+void foo() {
+  if (10 > 1.0)
+log("oh no!");
+  else
+log("ok");
+}
+  )cc";
+  const std::string Expected = R"(
+void log(const char* msg);
+void foo() {
+  if(!(10 > 1.0)) log("ok"); else log("oh no!");
+}
+  )";
+  EXPECT_EQ(Expected, test::runCheckOnCode(Input));
+}
+} // namespace
+} // namespace utils
+} // namespace tidy
+} // namespace clang
Index: clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
===
--- clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
+++ clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
@@ -17,6 +17,7 @@
   OverlappingReplacementsTest.cpp
   UsingInserterTest.cpp
   ReadabilityModuleTest.cpp
+  TransformerTidyTest.cpp
   )
 
 target_link_libraries(ClangTidyTests
@@ -36,4 +37,5 @@
   clangTidyUtils
   clangTooling
   clangToolingCore
+  clangToolingRefactor
   )
Index: clang-tools-extra/clang-tidy/utils/TransformerTidy.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/utils/TransformerTidy.h
@@ -0,0 +1,49 @@
+//===-- TransformerTidy.h - clang-tidy ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_TRANSFORMER_TIDY_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_TRANSFORMER_TIDY_H
+
+#include "../ClangTidy.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Tooling/Refactoring/Transformer.h"
+#include 
+#include 
+
+namespace clang {
+namespace tidy {
+namespace utils {
+
+// A base class for defining a ClangTidy check based on a rewrite rule.
+//
+// For example, given a RewriteRule `MyCheckAsRewriteRule`, one can define your
+// tidy check as follows:
+//
+// class MyTidyCheck : public TransformerTidy {
+//  public:
+//   MyTidyCheck(StringRef Name, ClangTidyContext *Context)
+//   : TransformerTidy(MyCheckAsRewriteRule, Name, Context) {}
+// };
+class TransformerTidy : public ClangTidyCheck {
+public:
+  TransformerTidy(tooling::RewriteRule R, StringRef Name,
+  ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context), Rule(std::move(R)) {}
+
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+

[PATCH] D61386: [clang-tidy] Add support writing a check as a Transformer rewrite rule.

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/TransformerTidy.cpp:51
+  DiagnosticBuilder Diag = diag(RootLoc, Message);
+  for (const tooling::Transformation &T : *Transformations) {
+Diag << FixItHint::CreateReplacement(T.Range, T.Replacement);

It's iterator, so auto could be used instead of type.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61386



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61386: [clang-tidy] Add support writing a check as a Transformer rewrite rule.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/clang-tidy/utils/TransformerTidy.cpp:22
+  // Verify the existence and validity of the AST node that roots this rule.
+  auto &NodesMap = Result.Nodes.getMap();
+  auto Root = NodesMap.find(tooling::RewriteRule::RootId);

Eugene.Zelenko wrote:
> Please don't use auto when return type is not spelled at same statement or 
> iterator. Same for other places. 
I left only the iterator's auto, expanded the rest.  Let me know if I've 
converted too many.   I wonder about this one here, for example, because the 
type of the map is essentially an internal detail of BoundNodes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61386



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61386: [clang-tidy] Add support writing a check as a Transformer rewrite rule.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 197589.
ymandel added a comment.

Reduced use of auto.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61386

Files:
  clang-tools-extra/clang-tidy/utils/CMakeLists.txt
  clang-tools-extra/clang-tidy/utils/TransformerTidy.cpp
  clang-tools-extra/clang-tidy/utils/TransformerTidy.h
  clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
  clang-tools-extra/unittests/clang-tidy/TransformerTidyTest.cpp

Index: clang-tools-extra/unittests/clang-tidy/TransformerTidyTest.cpp
===
--- /dev/null
+++ clang-tools-extra/unittests/clang-tidy/TransformerTidyTest.cpp
@@ -0,0 +1,63 @@
+//=== TransformerTidyTest.cpp - clang-tidy ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "../clang-tidy/utils/TransformerTidy.h"
+#include "ClangTidyTest.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Refactoring/Stencil.h"
+#include "clang/Tooling/Refactoring/Transformer.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tidy {
+namespace utils {
+namespace {
+// Invert the code of an if-statement, while maintaining its semantics.
+tooling::RewriteRule invertIf() {
+  using namespace ::clang::ast_matchers;
+  using tooling::change;
+  using tooling::stencil::cat;
+  using tooling::stencil::node;
+  using tooling::stencil::sNode;
+
+  StringRef C = "C", "T", E = "E";
+  return tooling::makeRule(
+  ifStmt(hasCondition(expr().bind(C)), hasThen(stmt().bind(T)),
+ hasElse(stmt().bind(E))),
+  change(cat("if(!(", node(C), ")) ", sNode(E), " else ", sNode(T;
+}
+
+class IfInverterTidy : public TransformerTidy {
+public:
+  IfInverterTidy(StringRef Name, ClangTidyContext *Context)
+  : TransformerTidy(invertIf(), Name, Context) {}
+};
+
+// Basic test of using a rewrite rule as a ClangTidy.
+TEST(TransformerTidyTest, Basic) {
+  const std::string Input = R"cc(
+void log(const char* msg);
+void foo() {
+  if (10 > 1.0)
+log("oh no!");
+  else
+log("ok");
+}
+  )cc";
+  const std::string Expected = R"(
+void log(const char* msg);
+void foo() {
+  if(!(10 > 1.0)) log("ok"); else log("oh no!");
+}
+  )";
+  EXPECT_EQ(Expected, test::runCheckOnCode(Input));
+}
+} // namespace
+} // namespace utils
+} // namespace tidy
+} // namespace clang
Index: clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
===
--- clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
+++ clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
@@ -17,6 +17,7 @@
   OverlappingReplacementsTest.cpp
   UsingInserterTest.cpp
   ReadabilityModuleTest.cpp
+  TransformerTidyTest.cpp
   )
 
 target_link_libraries(ClangTidyTests
@@ -36,4 +37,5 @@
   clangTidyUtils
   clangTooling
   clangToolingCore
+  clangToolingRefactor
   )
Index: clang-tools-extra/clang-tidy/utils/TransformerTidy.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/utils/TransformerTidy.h
@@ -0,0 +1,49 @@
+//===-- TransformerTidy.h - clang-tidy ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_TRANSFORMER_TIDY_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_TRANSFORMER_TIDY_H
+
+#include "../ClangTidy.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Tooling/Refactoring/Transformer.h"
+#include 
+#include 
+
+namespace clang {
+namespace tidy {
+namespace utils {
+
+// A base class for defining a ClangTidy check based on a rewrite rule.
+//
+// For example, given a RewriteRule `MyCheckAsRewriteRule`, one can define your
+// tidy check as follows:
+//
+// class MyTidyCheck : public TransformerTidy {
+//  public:
+//   MyTidyCheck(StringRef Name, ClangTidyContext *Context)
+//   : TransformerTidy(MyCheckAsRewriteRule, Name, Context) {}
+// };
+class TransformerTidy : public ClangTidyCheck {
+public:
+  TransformerTidy(tooling::RewriteRule R, StringRef Name,
+  ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context), Rule(std::move(R)) {}
+
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  tooling::RewriteRule Rule;
+};
+
+} //

[PATCH] D61386: [clang-tidy] Add support writing a check as a Transformer rewrite rule.

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/TransformerTidy.cpp:22
+  // Verify the existence and validity of the AST node that roots this rule.
+  auto &NodesMap = Result.Nodes.getMap();
+  auto Root = NodesMap.find(tooling::RewriteRule::RootId);

Please don't use auto when return type is not spelled at same statement or 
iterator. Same for other places. 



Comment at: clang-tools-extra/unittests/clang-tidy/TransformerTidyTest.cpp:10
+#include "../clang-tidy/utils/TransformerTidy.h"
+
+#include "ClangTidyTest.h"

Unnecessary empty line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61386



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61386: [clang-tidy] Add support writing a check as a Transformer rewrite rule.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel created this revision.
ymandel added a reviewer: aaron.ballman.
Herald added subscribers: cfe-commits, xazax.hun, mgorny.
Herald added a project: clang.

This revision introduces an adaptor from Transformer's rewrite rules
(`clang::tooling::RewriteRule`) to `ClangTidyCheck`.  For example, given a
RewriteRule `MyCheckAsRewriteRule`, it lets one define a tidy check as follows:

  class MyTidyCheck : public TransformerTidy {
   public:
MyTidyCheck(StringRef Name, ClangTidyContext *Context)
: TransformerTidy(MyCheckAsRewriteRule, Name, Context) {}
  };


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D61386

Files:
  clang-tools-extra/clang-tidy/utils/CMakeLists.txt
  clang-tools-extra/clang-tidy/utils/TransformerTidy.cpp
  clang-tools-extra/clang-tidy/utils/TransformerTidy.h
  clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
  clang-tools-extra/unittests/clang-tidy/TransformerTidyTest.cpp

Index: clang-tools-extra/unittests/clang-tidy/TransformerTidyTest.cpp
===
--- /dev/null
+++ clang-tools-extra/unittests/clang-tidy/TransformerTidyTest.cpp
@@ -0,0 +1,64 @@
+//=== TransformerTidyTest.cpp - clang-tidy ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "../clang-tidy/utils/TransformerTidy.h"
+
+#include "ClangTidyTest.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Refactoring/Stencil.h"
+#include "clang/Tooling/Refactoring/Transformer.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tidy {
+namespace utils {
+namespace {
+// Invert the code of an if-statement, while maintaining its semantics.
+tooling::RewriteRule invertIf() {
+  using namespace ::clang::ast_matchers;
+  using tooling::change;
+  using tooling::stencil::cat;
+  using tooling::stencil::node;
+  using tooling::stencil::sNode;
+
+  StringRef C = "C", "T", E = "E";
+  return tooling::makeRule(
+  ifStmt(hasCondition(expr().bind(C)), hasThen(stmt().bind(T)),
+ hasElse(stmt().bind(E))),
+  change(cat("if(!(", node(C), ")) ", sNode(E), " else ", sNode(T;
+}
+
+class IfInverterTidy : public TransformerTidy {
+public:
+  IfInverterTidy(StringRef Name, ClangTidyContext *Context)
+  : TransformerTidy(invertIf(), Name, Context) {}
+};
+
+// Basic test of using a rewrite rule as a ClangTidy.
+TEST(TransformerTidyTest, Basic) {
+  const std::string Input = R"cc(
+void log(const char* msg);
+void foo() {
+  if (10 > 1.0)
+log("oh no!");
+  else
+log("ok");
+}
+  )cc";
+  const std::string Expected = R"(
+void log(const char* msg);
+void foo() {
+  if(!(10 > 1.0)) log("ok"); else log("oh no!");
+}
+  )";
+  EXPECT_EQ(Expected, test::runCheckOnCode(Input));
+}
+} // namespace
+} // namespace utils
+} // namespace tidy
+} // namespace clang
Index: clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
===
--- clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
+++ clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
@@ -17,6 +17,7 @@
   OverlappingReplacementsTest.cpp
   UsingInserterTest.cpp
   ReadabilityModuleTest.cpp
+  TransformerTidyTest.cpp
   )
 
 target_link_libraries(ClangTidyTests
@@ -36,4 +37,5 @@
   clangTidyUtils
   clangTooling
   clangToolingCore
+  clangToolingRefactor
   )
Index: clang-tools-extra/clang-tidy/utils/TransformerTidy.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/utils/TransformerTidy.h
@@ -0,0 +1,49 @@
+//===-- TransformerTidy.h - clang-tidy ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_TRANSFORMER_TIDY_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_TRANSFORMER_TIDY_H
+
+#include "../ClangTidy.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Tooling/Refactoring/Transformer.h"
+#include 
+#include 
+
+namespace clang {
+namespace tidy {
+namespace utils {
+
+// A base class for defining a ClangTidy check based on a rewrite rule.
+//
+// For example, given a RewriteRule `MyCheckAsRewriteRule`, one can define your
+// tidy check as follows:
+//
+// class MyTidyCheck : public TransformerTidy {
+//  public:
+//   MyTidyCheck(StringRef Name, ClangTidyContext *Context)
+//   : TransformerTidy(MyCheckAsRewriteRule, Name, Context) {}
+// };
+class Tra