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

Yitzhak Mandelbaum via cfe-commits cfe-commits at lists.llvm.org
Fri May 24 10:16:03 PDT 2019


looking now...

On Fri, May 24, 2019 at 12:49 PM Ilya Biryukov <ibiryukov at google.com> wrote:

> 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/d75b3bde/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 4847 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190524/d75b3bde/attachment-0001.bin>


More information about the cfe-commits mailing list