[PATCH] D62340: [clang-tidy] In TransformerClangTidyCheck, require Explanation field.
Yitzhak Mandelbaum via cfe-commits
cfe-commits at lists.llvm.org
Fri May 24 10:36:04 PDT 2019
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/f66714ba/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/f66714ba/attachment-0001.bin>
More information about the cfe-commits
mailing list