[PATCH] D62340: [clang-tidy] In TransformerClangTidyCheck, require Explanation field.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

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

updated to use new version of `makeRule`; changed explanation text in test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62340

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


Index: clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
===================================================================
--- clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
+++ clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
@@ -26,15 +26,18 @@
   using tooling::change;
   using tooling::node;
   using tooling::statement;
+  using tooling::text;
   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))));
+  RewriteRule Rule = 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))),
+      text("negate condition and reverse `then` and `else` branches"));
+  return Rule;
 }
 
 class IfInverterCheck : public TransformerClangTidyCheck {
Index: clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h
===================================================================
--- clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h
+++ clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h
@@ -31,10 +31,14 @@
 // };
 class TransformerClangTidyCheck : public ClangTidyCheck {
 public:
+  // All cases in \p R must have a non-null \c Explanation, even though \c
+  // Explanation is optional for RewriteRule in general. Because the primary
+  // purpose of clang-tidy checks is to provide users with diagnostics, we
+  // assume that a missing explanation is a bug.  If no explanation is desired,
+  // indicate that explicitly (for example, by passing `text("no explanation")`
+  //  to `makeRule` as the `Explanation` argument).
   TransformerClangTidyCheck(tooling::RewriteRule R, StringRef Name,
-                            ClangTidyContext *Context)
-      : ClangTidyCheck(Name, Context), Rule(std::move(R)) {}
-
+                            ClangTidyContext *Context);
   void registerMatchers(ast_matchers::MatchFinder *Finder) final;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) final;
 
Index: clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
+++ clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
@@ -13,6 +13,17 @@
 namespace utils {
 using tooling::RewriteRule;
 
+TransformerClangTidyCheck::TransformerClangTidyCheck(tooling::RewriteRule R,
+                                                     StringRef Name,
+                                                     ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context), Rule(std::move(R)) {
+  for (const auto &Case : Rule.Cases) {
+    assert(Case.Explanation != nullptr &&
+           "clang-tidy checks must have an explanation by default;"
+           " explicitly provide an empty explanation if none is desired");
+  }
+}
+
 void TransformerClangTidyCheck::registerMatchers(
     ast_matchers::MatchFinder *Finder) {
   Finder->addDynamicMatcher(tooling::detail::buildMatcher(Rule), this);
@@ -44,15 +55,13 @@
   if (Transformations->empty())
     return;
 
-  StringRef Message = "no explanation";
-  if (Case.Explanation) {
-    if (Expected<std::string> E = Case.Explanation(Result))
-      Message = *E;
-    else
-      llvm::errs() << "Error in explanation: " << llvm::toString(E.takeError())
-                   << "\n";
+  Expected<std::string> Explanation = Case.Explanation(Result);
+  if (!Explanation) {
+    llvm::errs() << "Error in explanation: "
+                 << llvm::toString(Explanation.takeError()) << "\n";
+    return;
   }
-  DiagnosticBuilder Diag = diag(RootLoc, Message);
+  DiagnosticBuilder Diag = diag(RootLoc, *Explanation);
   for (const auto &T : *Transformations) {
     Diag << FixItHint::CreateReplacement(T.Range, T.Replacement);
   }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D62340.201256.patch
Type: text/x-patch
Size: 4365 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190524/576ab98c/attachment-0001.bin>