[PATCH] D62340: [clang-tidy] In TransformerClangTidyCheck, require Explanation field.
Yitzhak Mandelbaum via cfe-commits
cfe-commits at lists.llvm.org
Fri May 24 11:08:33 PDT 2019
version 1 here: https://reviews.llvm.org/D62412
On Fri, May 24, 2019 at 1:36 PM Yitzhak Mandelbaum <yitzhakm at google.com>
wrote:
> So, it only shows up in Release build, I assume because the assert is left
> out:
> 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");
> }
> I can think of a few solutions, please let me know if there's a
> standard/preferred approach:
>
> 1. Use std::all_of.
> assert(std::all_of(Rule.Cases.cbegin(), Rule.Cases.cend(), [](const
> RewriteRule::Case &C) { return C.Explanation != nullptr; }
> && ...);
>
> 2. Trivially use the variable.
> I could add a trivial use to the loop
> for (const auto &Case : Rule.Cases) {
> + (void)Case;
> assert(Case.Explanation != nullptr &&
> "clang-tidy checks must have an explanation by default;"
> " explicitly provide an empty explanation if none is desired");
> }
>
> 3. Use llvm_unreachable instead.
> Rewrite the assert to
> if (Case.Explanation == nullptr)
> llvm_unreachable(...)
>
> On Fri, May 24, 2019 at 1:16 PM Yitzhak Mandelbaum <yitzhakm at google.com>
> wrote:
>
>> 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/ce86e4cf/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/ce86e4cf/attachment-0001.bin>
More information about the cfe-commits
mailing list