<div dir="ltr"><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">looking now...<br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, May 24, 2019 at 12:49 PM Ilya Biryukov <<a href="mailto:ibiryukov@google.com">ibiryukov@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">This seems to produce warnings about unused variables:<div>…/llvm/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp:20:20: warning: unused variable 'Case' [-Wunused-variable] <br> for (const auto &Case : Rule.Cases) {<br></div><div><br></div><div>Could you take a look?</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, May 24, 2019 at 6:29 PM Yitzhak Mandelbaum via Phabricator <<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">This revision was automatically updated to reflect the committed changes.<br>
Closed by commit rL361647: [clang-tidy] In TransformerClangTidyCheck, require Explanation field. (authored by ymandel, committed by ).<br>
Herald added a project: LLVM.<br>
Herald added a subscriber: llvm-commits.<br>
<br>
Changed prior to commit:<br>
<a href="https://reviews.llvm.org/D62340?vs=201256&id=201272#toc" rel="noreferrer" target="_blank">https://reviews.llvm.org/D62340?vs=201256&id=201272#toc</a><br>
<br>
Repository:<br>
rL LLVM<br>
<br>
CHANGES SINCE LAST ACTION<br>
<a href="https://reviews.llvm.org/D62340/new/" rel="noreferrer" target="_blank">https://reviews.llvm.org/D62340/new/</a><br>
<br>
<a href="https://reviews.llvm.org/D62340" rel="noreferrer" target="_blank">https://reviews.llvm.org/D62340</a><br>
<br>
Files:<br>
clang-tools-extra/trunk/clang-tidy/utils/TransformerClangTidyCheck.cpp<br>
clang-tools-extra/trunk/clang-tidy/utils/TransformerClangTidyCheck.h<br>
clang-tools-extra/trunk/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp<br>
<br>
<br>
Index: clang-tools-extra/trunk/clang-tidy/utils/TransformerClangTidyCheck.h<br>
===================================================================<br>
--- clang-tools-extra/trunk/clang-tidy/utils/TransformerClangTidyCheck.h<br>
+++ clang-tools-extra/trunk/clang-tidy/utils/TransformerClangTidyCheck.h<br>
@@ -31,10 +31,14 @@<br>
// };<br>
class TransformerClangTidyCheck : public ClangTidyCheck {<br>
public:<br>
+ // All cases in \p R must have a non-null \c Explanation, even though \c<br>
+ // Explanation is optional for RewriteRule in general. Because the primary<br>
+ // purpose of clang-tidy checks is to provide users with diagnostics, we<br>
+ // assume that a missing explanation is a bug. If no explanation is desired,<br>
+ // indicate that explicitly (for example, by passing `text("no explanation")`<br>
+ // to `makeRule` as the `Explanation` argument).<br>
TransformerClangTidyCheck(tooling::RewriteRule R, StringRef Name,<br>
- ClangTidyContext *Context)<br>
- : ClangTidyCheck(Name, Context), Rule(std::move(R)) {}<br>
-<br>
+ ClangTidyContext *Context);<br>
void registerMatchers(ast_matchers::MatchFinder *Finder) final;<br>
void check(const ast_matchers::MatchFinder::MatchResult &Result) final;<br>
<br>
Index: clang-tools-extra/trunk/clang-tidy/utils/TransformerClangTidyCheck.cpp<br>
===================================================================<br>
--- clang-tools-extra/trunk/clang-tidy/utils/TransformerClangTidyCheck.cpp<br>
+++ clang-tools-extra/trunk/clang-tidy/utils/TransformerClangTidyCheck.cpp<br>
@@ -13,6 +13,17 @@<br>
namespace utils {<br>
using tooling::RewriteRule;<br>
<br>
+TransformerClangTidyCheck::TransformerClangTidyCheck(tooling::RewriteRule R,<br>
+ StringRef Name,<br>
+ ClangTidyContext *Context)<br>
+ : ClangTidyCheck(Name, Context), Rule(std::move(R)) {<br>
+ for (const auto &Case : Rule.Cases) {<br>
+ assert(Case.Explanation != nullptr &&<br>
+ "clang-tidy checks must have an explanation by default;"<br>
+ " explicitly provide an empty explanation if none is desired");<br>
+ }<br>
+}<br>
+<br>
void TransformerClangTidyCheck::registerMatchers(<br>
ast_matchers::MatchFinder *Finder) {<br>
Finder->addDynamicMatcher(tooling::detail::buildMatcher(Rule), this);<br>
@@ -44,15 +55,13 @@<br>
if (Transformations->empty())<br>
return;<br>
<br>
- StringRef Message = "no explanation";<br>
- if (Case.Explanation) {<br>
- if (Expected<std::string> E = Case.Explanation(Result))<br>
- Message = *E;<br>
- else<br>
- llvm::errs() << "Error in explanation: " << llvm::toString(E.takeError())<br>
- << "\n";<br>
+ Expected<std::string> Explanation = Case.Explanation(Result);<br>
+ if (!Explanation) {<br>
+ llvm::errs() << "Error in explanation: "<br>
+ << llvm::toString(Explanation.takeError()) << "\n";<br>
+ return;<br>
}<br>
- DiagnosticBuilder Diag = diag(RootLoc, Message);<br>
+ DiagnosticBuilder Diag = diag(RootLoc, *Explanation);<br>
for (const auto &T : *Transformations) {<br>
Diag << FixItHint::CreateReplacement(T.Range, T.Replacement);<br>
}<br>
Index: clang-tools-extra/trunk/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp<br>
===================================================================<br>
--- clang-tools-extra/trunk/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp<br>
+++ clang-tools-extra/trunk/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp<br>
@@ -26,15 +26,18 @@<br>
using tooling::change;<br>
using tooling::node;<br>
using tooling::statement;<br>
+ using tooling::text;<br>
using tooling::stencil::cat;<br>
<br>
StringRef C = "C", T = "T", E = "E";<br>
- return tooling::makeRule(ifStmt(hasCondition(expr().bind(C)),<br>
- hasThen(stmt().bind(T)),<br>
- hasElse(stmt().bind(E))),<br>
- change(statement(RewriteRule::RootID),<br>
- cat("if(!(", node(C), ")) ", statement(E),<br>
- " else ", statement(T))));<br>
+ RewriteRule Rule = tooling::makeRule(<br>
+ ifStmt(hasCondition(expr().bind(C)), hasThen(stmt().bind(T)),<br>
+ hasElse(stmt().bind(E))),<br>
+ change(<br>
+ statement(RewriteRule::RootID),<br>
+ cat("if(!(", node(C), ")) ", statement(E), " else ", statement(T))),<br>
+ text("negate condition and reverse `then` and `else` branches"));<br>
+ return Rule;<br>
}<br>
<br>
class IfInverterCheck : public TransformerClangTidyCheck {<br>
<br>
<br>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="gmail-m_2007650560515508534gmail_signature"><div dir="ltr"><div><div dir="ltr"><div>Regards,</div><div>Ilya Biryukov</div></div></div></div></div>
</blockquote></div>