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

[Ilya Biryukov via cfe-commits]

This seems to produce warnings about unused variables:
…/llvm/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp:20:20:
warning: unused variable 'Case' [-Wunused-variable]

  for (const auto &Case : Rule.Cases) {

Could you take a look?

On Fri, May 24, 2019 at 6:29 PM Yitzhak Mandelbaum via Phabricator <
reviews at reviews.llvm.org> wrote:

> This revision was automatically updated to reflect the committed changes.
> Closed by commit rL361647: [clang-tidy] In TransformerClangTidyCheck,
> require Explanation field. (authored by ymandel, committed by ).
> Herald added a project: LLVM.
> Herald added a subscriber: llvm-commits.
>
> Changed prior to commit:
>   https://reviews.llvm.org/D62340?vs=201256&id=201272#toc
>
> Repository:
>   rL LLVM
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D62340/new/
>
> https://reviews.llvm.org/D62340
>
> Files:
>   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/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
> @@ -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/trunk/clang-tidy/utils/TransformerClangTidyCheck.cpp
> ===================================================================
> --- clang-tools-extra/trunk/clang-tidy/utils/TransformerClangTidyCheck.cpp
> +++ clang-tools-extra/trunk/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);
>    }
> Index:
> clang-tools-extra/trunk/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
> ===================================================================
> ---
> clang-tools-extra/trunk/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
> +++
> clang-tools-extra/trunk/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 {
>
>
>

-- 
Regards,
Ilya Biryukov
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190524/38d1e2f1/attachment.html>