<div dir="ltr"><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">version 1 here: <a href="https://reviews.llvm.org/D62412" rel="noreferrer" target="_blank" style="font-family:Arial,Helvetica,sans-serif">https://reviews.llvm.org/D62412</a><br></div><div class="gmail-gs" style="margin:0px;padding:0px 0px 20px;width:1455.02px;font-family:Roboto,RobotoDraft,Helvetica,Arial,sans-serif;font-size:medium"><div class="gmail-"><div id="gmail-:1ro" class="gmail-ii gmail-gt" style="font-size:12.8px;direction:ltr;margin:8px 0px 0px;padding:0px"><div id="gmail-:1uf" class="gmail-a3s gmail-aXjCH" style="overflow:hidden;font-variant-numeric:normal;font-variant-east-asian:normal;font-stretch:normal;font-size:small;line-height:1.5;font-family:Arial,Helvetica,sans-serif"><div class="gmail-yj6qo"></div><div class="gmail-adL"><br><br><br><br></div></div></div><div class="gmail-hi" style="border-bottom-left-radius:1px;border-bottom-right-radius:1px;padding:0px;width:auto;background:rgb(242,242,242);margin:0px"></div></div></div><br class="gmail-Apple-interchange-newline"></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, May 24, 2019 at 1:36 PM Yitzhak Mandelbaum <<a href="mailto:yitzhakm@google.com">yitzhakm@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"><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">So, it only shows up in Release build, I assume because the assert is left out:</div> 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> }<div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">I can think of a few solutions, please let me know if there's a standard/preferred approach:</div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"><br></div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">1. Use std::all_of.</div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">assert(std::all_of(Rule.Cases.cbegin(), Rule.Cases.cend(), [](const RewriteRule::Case &C) { return C.Explanation != nullptr; }</div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"> && ...);</div><br></div><div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">2. Trivially use the variable.</div><div><span style="font-family:arial,helvetica,sans-serif">I could add a trivial use to the loop</span><br></div><div> for (const auto &Case : Rule.Cases) {<br><span class="gmail_default" style="font-family:arial,helvetica,sans-serif">+</span><span class="gmail_default" style="font-family:arial,helvetica,sans-serif"> </span><span style="font-family:arial,helvetica,sans-serif">(void)Case;</span></div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"></div><div><span class="gmail_default" style="font-family:arial,helvetica,sans-serif"></span> 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><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"><br></div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">3. Use llvm_unreachable instead.</div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">Rewrite the assert to</div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">if (Case.Explanation == nullptr)</div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"> llvm_unreachable(...)<br></div></div></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, May 24, 2019 at 1:16 PM Yitzhak Mandelbaum <<a href="mailto:yitzhakm@google.com" target="_blank">yitzhakm@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"><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" target="_blank">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_6448314910674013862gmail-m_-5693801541676899897gmail-m_2007650560515508534gmail_signature"><div dir="ltr"><div><div dir="ltr"><div>Regards,</div><div>Ilya Biryukov</div></div></div></div></div>
</blockquote></div>
</blockquote></div>
</blockquote></div>