Author: ymandel Date: Fri May 24 09:32:03 2019 New Revision: 361647 URL: http://llvm.org/viewvc/llvm-project?rev=361647&view=rev Log: [clang-tidy] In TransformerClangTidyCheck, require Explanation field.
Summary: In general, the `Explanation` field is optional in `RewriteRule` cases. But, because the primary purpose of clang-tidy checks is to provide users with diagnostics, we assume that a missing explanation is a bug. This change adds an assertion that checks all cases for an explanation, and updates the code to rely on that assertion correspondingly. Reviewers: ilya-biryukov Subscribers: xazax.hun, cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D62340 Modified: 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 Modified: clang-tools-extra/trunk/clang-tidy/utils/TransformerClangTidyCheck.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/utils/TransformerClangTidyCheck.cpp?rev=361647&r1=361646&r2=361647&view=diff ============================================================================== --- clang-tools-extra/trunk/clang-tidy/utils/TransformerClangTidyCheck.cpp (original) +++ clang-tools-extra/trunk/clang-tidy/utils/TransformerClangTidyCheck.cpp Fri May 24 09:32:03 2019 @@ -13,6 +13,17 @@ namespace tidy { 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 @@ void TransformerClangTidyCheck::check( 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); } Modified: clang-tools-extra/trunk/clang-tidy/utils/TransformerClangTidyCheck.h URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/utils/TransformerClangTidyCheck.h?rev=361647&r1=361646&r2=361647&view=diff ============================================================================== --- clang-tools-extra/trunk/clang-tidy/utils/TransformerClangTidyCheck.h (original) +++ clang-tools-extra/trunk/clang-tidy/utils/TransformerClangTidyCheck.h Fri May 24 09:32:03 2019 @@ -31,10 +31,14 @@ namespace utils { // }; 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; Modified: clang-tools-extra/trunk/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp?rev=361647&r1=361646&r2=361647&view=diff ============================================================================== --- clang-tools-extra/trunk/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp (original) +++ clang-tools-extra/trunk/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp Fri May 24 09:32:03 2019 @@ -26,15 +26,18 @@ RewriteRule invertIf() { 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 { _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
