llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tidy <details> <summary>Changes</summary> This check finds constants and function calls to math functions that can be replaced with c++20's mathematical constants ('numbers' header) and offers fixit-hints. Does not match the use of variables or macros with that value and instead, offers a replacement at the definition of said variables and macros. --- Patch is 27.80 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/66583.diff 8 Files Affected: - (modified) clang-tools-extra/clang-tidy/misc/CMakeLists.txt (+1) - (added) clang-tools-extra/clang-tidy/misc/MathConstantCheck.cpp (+377) - (added) clang-tools-extra/clang-tidy/misc/MathConstantCheck.h (+37) - (modified) clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp (+3) - (modified) clang-tools-extra/docs/ReleaseNotes.rst (+6) - (modified) clang-tools-extra/docs/clang-tidy/checks/list.rst (+1) - (added) clang-tools-extra/docs/clang-tidy/checks/misc/math-constant.rst (+25) - (added) clang-tools-extra/test/clang-tidy/checkers/misc/math-constant.cpp (+203) ``````````diff diff --git a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt index 2e88e68a5447825..9259de378340f45 100644 --- a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt @@ -22,6 +22,7 @@ add_clang_library(clangTidyMiscModule ConfusableIdentifierCheck.cpp HeaderIncludeCycleCheck.cpp IncludeCleanerCheck.cpp + MathConstantCheck.cpp MiscTidyModule.cpp MisleadingBidirectional.cpp MisleadingIdentifier.cpp diff --git a/clang-tools-extra/clang-tidy/misc/MathConstantCheck.cpp b/clang-tools-extra/clang-tidy/misc/MathConstantCheck.cpp new file mode 100644 index 000000000000000..5fb4c999cb4361d --- /dev/null +++ b/clang-tools-extra/clang-tidy/misc/MathConstantCheck.cpp @@ -0,0 +1,377 @@ +//===--- MathConstantCheck.cpp - clang_tidy -------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX_License_Identifier: Apache_2.0 WITH LLVM_exception +// +//===----------------------------------------------------------------------===// + +#include "MathConstantCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/AST/Expr.h" +#include "clang/AST/Stmt.h" +#include "clang/AST/Type.h" +#include "clang/ASTMatchers/ASTMatchers.h" +#include "clang/ASTMatchers/ASTMatchersInternal.h" +#include "clang/ASTMatchers/ASTMatchersMacros.h" +#include "clang/Basic/Diagnostic.h" +#include "clang/Basic/SourceLocation.h" +#include "clang/Basic/TokenKinds.h" +#include "clang/Lex/PPCallbacks.h" +#include "clang/Lex/Preprocessor.h" +#include "clang/Lex/Token.h" +#include "clang/Tooling/Transformer/RewriteRule.h" +#include "clang/Tooling/Transformer/Stencil.h" +#include "llvm/ADT/StringRef.h" +#include "llvm/Support/MathExtras.h" +#include <cstdint> +#include <string> + +namespace { +using namespace clang::ast_matchers; +using clang::ast_matchers::internal::Matcher; +using clang::transformer::addInclude; +using clang::transformer::applyFirst; +using clang::transformer::ASTEdit; +using clang::transformer::cat; +using clang::transformer::changeTo; +using clang::transformer::edit; +using clang::transformer::EditGenerator; +using clang::transformer::flattenVector; +using clang::transformer::RewriteRuleWith; +using llvm::StringRef; + +constexpr double Pi = 3.141592653589793238462643383279502884197169399; +constexpr double Euler = 2.718281828459045235360287471352662497757247093; +constexpr double Phi = 1.618033988749894848204586834365638117720309179; +constexpr double Egamma = 0.577215664901532860606512090082402431042159335; + +constexpr auto DiffThreshold = 0.001; + +AST_MATCHER_P2(clang::FloatingLiteral, near, double, Value, double, Threshold) { + return std::abs(Node.getValue().convertToDouble() - Value) < Threshold; +} + +// We don't want to match uses of macros, such as +// +// auto PiHalved = MY_PI / 2; +// +// because a project might use defines for math constants. +// Instead, the macro definition is matched and the value is exchanged there. +// Hinting at replacing macro definitions with language constructs is done in +// another check. +AST_MATCHER(clang::Expr, isMathMacro) { return Node.getBeginLoc().isMacroID(); } + +AST_MATCHER_P(clang::QualType, hasUnqualifiedDesugaredType, + Matcher<clang::QualType>, InnerMatcher) { + return InnerMatcher.matches(Node->getCanonicalTypeUnqualified(), Finder, + Builder); +} + +auto matchMathCall(const StringRef FunctionName, + const Matcher<clang::Expr> ArgumentMatcher) { + return callExpr(callee(functionDecl(hasName(FunctionName))), + hasArgument(0, ignoringImplicit(ArgumentMatcher))); +} + +auto matchSqrt(const Matcher<clang::Expr> ArgumentMatcher) { + return matchMathCall("sqrt", ArgumentMatcher); +} + +// 'MatchDeclRefExprOrMacro' is used to differentiate matching expressions where +// the value of anything used is near 'Val' and matching expressions where we +// only care about the actual literal. +// We don't want top-level matches to match a simple DeclRefExpr/macro that was +// initialized with this value because projects might declare their own +// constants (e.g. namespaced constants or macros) to be used. We don't want to +// flag the use of these variables/constants, but modify the definition of the +// variable or macro. +// +// example: +// const auto e = 2.71828182; // std::numbers::e +// ^^^^^^^^^^ +// match here +// +// auto use = e / 2; +// ^ +// don't match this as a top-level match, this would create noise +// +// auto use2 = log2(e); // std::numbers::log2e +// ^^^^^^^ +// match here, matcher needs to check the initialization +// of e to match log2e +// +// Therefore, all top-level matcher set MatchDeclRefExprOrMacro to false +auto matchFloatValueNear(const double Val, + const bool MatchDeclRefExprOrMacro = true) { + const auto FloatVal = floatLiteral(near(Val, DiffThreshold)); + if (!MatchDeclRefExprOrMacro) { + return expr(unless(isMathMacro()), ignoringImplicit(FloatVal)); + } + + const auto Dref = declRefExpr(to(varDecl( + anyOf(isConstexpr(), varDecl(hasType(qualType(isConstQualified())))), + hasInitializer(FloatVal)))); + return expr(ignoringImplicit(anyOf(FloatVal, Dref))); +} + +auto matchValue(const int64_t ValInt) { + const auto Int2 = integerLiteral(equals(ValInt)); + const auto Float2 = matchFloatValueNear(static_cast<double>(ValInt)); + const auto Dref = declRefExpr(to(varDecl( + anyOf(isConstexpr(), varDecl(hasType(qualType(isConstQualified())))), + hasInitializer(expr(ignoringImplicit(anyOf(Int2, Float2))))))); + return expr(ignoringImplicit(anyOf(Int2, Float2, Dref))); +} + +auto match1Div(const Matcher<clang::Expr> Match) { + return binaryOperator(hasOperatorName("/"), hasLHS(matchValue(1)), + hasRHS(ignoringImplicit(Match))); +} + +auto matchEuler() { + return expr( + anyOf(matchFloatValueNear(Euler), matchMathCall("exp", matchValue(1)))); +} +auto matchEulerTopLevel() { + return expr(anyOf(matchFloatValueNear(Euler, false), + matchMathCall("exp", matchValue(1)))); +} + +auto matchLog2Euler() { + return expr(anyOf(matchFloatValueNear(llvm::numbers::log2e, false), + matchMathCall("log2", matchEuler()))); +} + +auto matchLog10Euler() { + return expr(anyOf(matchFloatValueNear(llvm::numbers::log10e, false), + matchMathCall("log10", matchEuler()))); +} + +auto matchPi() { return matchFloatValueNear(Pi); } +auto matchPiTopLevel() { return matchFloatValueNear(Pi, false); } + +auto matchEgamma() { return matchFloatValueNear(Egamma, false); } + +auto matchInvPi() { + return expr( + anyOf(matchFloatValueNear(llvm::numbers::inv_pi), match1Div(matchPi()))); +} + +auto matchInvSqrtPi() { + return expr(anyOf(matchFloatValueNear(llvm::numbers::inv_sqrtpi, false), + match1Div(matchSqrt(matchPi())))); +} + +auto matchLn2() { + return expr(anyOf(matchFloatValueNear(llvm::numbers::ln2, false), + matchMathCall("log", ignoringImplicit(matchValue(2))))); +} + +auto machterLn10() { + return expr(anyOf(matchFloatValueNear(llvm::numbers::ln10, false), + matchMathCall("log", ignoringImplicit(matchValue(10))))); +} + +auto matchSqrt2() { + return expr(anyOf(matchFloatValueNear(llvm::numbers::sqrt2, false), + matchSqrt(matchValue(2)))); +} + +auto matchSqrt3() { + return expr(anyOf(matchFloatValueNear(llvm::numbers::sqrt3, false), + matchSqrt(matchValue(3)))); +} + +auto matchInvSqrt3() { + return expr(anyOf(matchFloatValueNear(llvm::numbers::inv_sqrt3, false), + match1Div(matchSqrt(matchValue(3))))); +} + +auto matchPhi() { + const auto PhiFormula = binaryOperator( + hasOperatorName("/"), + hasLHS(parenExpr(has(binaryOperator( + hasOperatorName("+"), + hasEitherOperand(matchMathCall("sqrt", matchValue(5))))))), + hasRHS(matchValue(2))); + return expr(anyOf(PhiFormula, matchFloatValueNear(Phi))); +} + +EditGenerator +chainedIfBound(ASTEdit DefaultEdit, + llvm::SmallVector<std::pair<StringRef, ASTEdit>, 2> Edits) { + return [Edits = std::move(Edits), DefaultEdit = std::move(DefaultEdit)]( + const MatchFinder::MatchResult &Result) { + auto &Map = Result.Nodes.getMap(); + for (const auto &[Id, EditOfId] : Edits) { + if (Map.find(Id) != Map.end()) { + return edit(EditOfId)(Result); + } + } + return edit(DefaultEdit)(Result); + }; +} + +RewriteRuleWith<std::string> makeRule(const Matcher<clang::Stmt> Matcher, + const StringRef Constant) { + static const auto AddNumbersInclude = + addInclude("numbers", clang::transformer::IncludeFormat::Angled); + + const auto DefaultEdit = changeTo(cat("std::numbers::", Constant)); + const auto FloatEdit = changeTo(cat("std::numbers::", Constant, "_v<float>")); + const auto LongDoubleEdit = + changeTo(cat("std::numbers::", Constant, "_v<long double>")); + + const auto EditRules = chainedIfBound( + DefaultEdit, {{"float", FloatEdit}, {"long double", LongDoubleEdit}}); + + return makeRule( + expr(Matcher, + hasType(qualType(hasCanonicalType(hasUnqualifiedDesugaredType(anyOf( + qualType(asString("float")).bind("float"), + qualType(asString("double")), + qualType(asString("long double")).bind("long double"))))))), + flattenVector({edit(AddNumbersInclude), EditRules}), + cat("prefer math constant")); +} + +/* + List of all math constants + + e + + log2e + + log10e + + pi + + inv_pi + + inv_sqrtpi + + ln2 + + ln10 + + sqrt2 + + sqrt3 + + inv_sqrt3 + + egamma + + phi +*/ + +RewriteRuleWith<std::string> makeRewriteRule() { + return applyFirst({ + makeRule(matchLog2Euler(), "log2e"), + makeRule(matchLog10Euler(), "log10e"), + makeRule(matchEulerTopLevel(), "e"), + makeRule(matchEgamma(), "egamma"), + makeRule(matchInvSqrtPi(), "inv_sqrtpi"), + makeRule(matchInvPi(), "inv_pi"), + makeRule(matchPiTopLevel(), "pi"), + makeRule(matchLn2(), "ln2"), + makeRule(machterLn10(), "ln10"), + makeRule(matchSqrt2(), "sqrt2"), + makeRule(matchInvSqrt3(), "inv_sqrt3"), + makeRule(matchSqrt3(), "sqrt3"), + makeRule(matchPhi(), "phi"), + }); +} + +class MathConstantMacroCallback : public clang::PPCallbacks { +public: + explicit MathConstantMacroCallback( + clang::tidy::misc::MathConstantCheck *Check) + : Check{Check} {}; + + void MacroDefined(const clang::Token & /*MacroNameTok*/, + const clang::MacroDirective *MD) override { + for (const auto &Tok : MD->getDefinition().getMacroInfo()->tokens()) { + if (!Tok.is(clang::tok::numeric_constant)) { + continue; + } + + const auto Definition = + llvm::StringRef{Tok.getLiteralData(), Tok.getLength()}; + double Value{}; + Definition.getAsDouble(Value); + + const auto IsNear = [](const auto Lhs, const auto Rhs) { + return std::abs(Lhs - Rhs) < DiffThreshold; + }; + + if (IsNear(Value, llvm::numbers::log2e)) { + reportDiag(Tok, "std::numbers::log2e"); + return; + } + if (IsNear(Value, llvm::numbers::log10e)) { + reportDiag(Tok, "std::numbers::log10e"); + return; + } + if (IsNear(Value, llvm::numbers::e)) { + reportDiag(Tok, "std::numbers::e"); + return; + } + if (IsNear(Value, llvm::numbers::egamma)) { + reportDiag(Tok, "std::numbers::egamma"); + return; + } + if (IsNear(Value, llvm::numbers::inv_sqrtpi)) { + reportDiag(Tok, "std::numbers::inv_sqrtpi"); + return; + } + if (IsNear(Value, llvm::numbers::inv_pi)) { + reportDiag(Tok, "std::numbers::inv_pi"); + return; + } + if (IsNear(Value, llvm::numbers::pi)) { + reportDiag(Tok, "std::numbers::pi"); + return; + } + if (IsNear(Value, llvm::numbers::ln2)) { + reportDiag(Tok, "std::numbers::ln2"); + return; + } + if (IsNear(Value, llvm::numbers::ln10)) { + reportDiag(Tok, "std::numbers::ln10"); + return; + } + if (IsNear(Value, llvm::numbers::sqrt2)) { + reportDiag(Tok, "std::numbers::sqrt2"); + return; + } + if (IsNear(Value, llvm::numbers::inv_sqrt3)) { + reportDiag(Tok, "std::numbers::inv_sqrt3"); + return; + } + if (IsNear(Value, llvm::numbers::sqrt3)) { + reportDiag(Tok, "std::numbers::sqrt3"); + return; + } + if (IsNear(Value, llvm::numbers::phi)) { + reportDiag(Tok, "std::numbers::phi"); + return; + } + } + } + +private: + void reportDiag(const clang::Token &Tok, const llvm::StringRef Constant) { + Check->diag(Tok.getLocation(), "prefer math constant") + << clang::FixItHint::CreateReplacement( + clang::SourceRange(Tok.getLocation(), Tok.getLastLoc()), + Constant); + } + + clang::tidy::misc::MathConstantCheck *Check{}; +}; +} // namespace + +namespace clang::tidy::misc { +MathConstantCheck::MathConstantCheck(const StringRef Name, + ClangTidyContext *const Context) + : TransformerClangTidyCheck(Name, Context) { + setRule(makeRewriteRule()); +} + +void MathConstantCheck::registerPPCallbacks(const SourceManager &SM, + Preprocessor *PP, + Preprocessor *ModuleExpanderPP) { + utils::TransformerClangTidyCheck::registerPPCallbacks(SM, PP, + ModuleExpanderPP); + PP->addPPCallbacks(std::make_unique<MathConstantMacroCallback>(this)); +} +} // namespace clang::tidy::misc diff --git a/clang-tools-extra/clang-tidy/misc/MathConstantCheck.h b/clang-tools-extra/clang-tidy/misc/MathConstantCheck.h new file mode 100644 index 000000000000000..3617e173290306d --- /dev/null +++ b/clang-tools-extra/clang-tidy/misc/MathConstantCheck.h @@ -0,0 +1,37 @@ +//===--- MathConstantCheck.h - clang-tidy -----------------------*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MATHCONSTANTCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MATHCONSTANTCHECK_H + +#include "../utils/TransformerClangTidyCheck.h" + +namespace clang::tidy::misc { + +/// Finds constants and function calls to math functions that can be replaced +/// with c++20's mathematical constants ('numbers' header). Does not match the +/// use of variables or macros with that value and instead offers a replacement +/// at the definition of said variables and macros. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/misc/math-constant.html +class MathConstantCheck : public utils::TransformerClangTidyCheck { +public: + MathConstantCheck(StringRef Name, ClangTidyContext *Context); + + bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { + return LangOpts.CPlusPlus20; + } + + void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP, + Preprocessor *ModuleExpanderPP) override; +}; + +} // namespace clang::tidy::misc + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MATHCONSTANTCHECK_H diff --git a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp index 92590506e1ec1e6..06beb0f8d399f9f 100644 --- a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp @@ -14,6 +14,7 @@ #include "DefinitionsInHeadersCheck.h" #include "HeaderIncludeCycleCheck.h" #include "IncludeCleanerCheck.h" +#include "MathConstantCheck.h" #include "MisleadingBidirectional.h" #include "MisleadingIdentifier.h" #include "MisplacedConstCheck.h" @@ -46,6 +47,8 @@ class MiscModule : public ClangTidyModule { CheckFactories.registerCheck<HeaderIncludeCycleCheck>( "misc-header-include-cycle"); CheckFactories.registerCheck<IncludeCleanerCheck>("misc-include-cleaner"); + CheckFactories.registerCheck<MathConstantCheck>( + "misc-math-constant"); CheckFactories.registerCheck<MisleadingBidirectionalCheck>( "misc-misleading-bidirectional"); CheckFactories.registerCheck<MisleadingIdentifierCheck>( diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 6d6f51998a01e57..ba6ce872c91b333 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -163,6 +163,12 @@ New checks Flags coroutines that suspend while a lock guard is in scope at the suspension point. +- New :doc:`misc-math-constant + <clang-tidy/checks/misc/math-constant>` check. + + Finds constants and function calls to math functions that can be replaced + with c++20's mathematical constants ('numbers' header). + - New :doc:`modernize-use-constraints <clang-tidy/checks/modernize/use-constraints>` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 1baabceea06ef48..02fe8d7b7122180 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -244,6 +244,7 @@ Clang-Tidy Checks :doc:`misc-definitions-in-headers <misc/definitions-in-headers>`, "Yes" :doc:`misc-header-include-cycle <misc/header-include-cycle>`, :doc:`misc-include-cleaner <misc/include-cleaner>`, "Yes" + :doc:`misc-math-constant <misc/math-constant>`, "Yes" :doc:`misc-misleading-bidirectional <misc/misleading-bidirectional>`, :doc:`misc-misleading-identifier <misc/misleading-identifier>`, :doc:`misc-misplaced-const <misc/misplaced-const>`, diff --git a/clang-tools-extra/docs/clang-tidy/checks/misc/math-constant.rst b/clang-tools-extra/docs/clang-tidy/checks/misc/math-constant.rst new file mode 100644 index 000000000000000..37837721ee2381d --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/misc/math-constant.rst @@ -0,0 +1,25 @@ +.. title:: clang-tidy - misc-math-constant + +misc-math-constant +================== + +This check finds constants and function calls to math functions that can be replaced +with c++20's mathematical constants ('numbers' header) and offers fixit-hints. +Does not match the use of variables or macros with that value and instead, offers a replacement +at the definition of said variables and macros. + +.. code-block:: c++ + double sqrt(double); + double log(double); + + #define MY_PI 3.1415926 // #define MY_PI std::numbers::pi + + void foo() { + const double Pi = 3.141592653589; // const double Pi = std::numbers::pi + const auto Use = Pi / 2; // no match for Pi + static constexpr double Euler = 2.7182818; // static constexpr double Euler = std::numbers::e; + + log2(exp(1)); // std::numbers::log2e; + log2(Euler); // std::numbers::log2e; + 1 / sqrt(MY_PI); // std::numbers::inv_sqrtpi; + } diff --git a/clang-tools-extra/test/clang-tidy/checkers/misc/math-constant.cpp b/clang-tools-extra/test/clang-tidy/checkers/misc/math-constant.cpp new file mode 100644 index 000000000000000..91fbb045b1afcef --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/misc/math-constant.cpp @@ -0,0 +1,203 @@ +// RUN: %check_clang_tidy -std=c++20 %s misc-math-constant %t + +// CHECK-FIXES: #include <numbers> + +namespace bar { + double sqrt(double Arg); + float sqrt(float Arg); + template <typename T> + auto sqrt(T val) { return sqrt(static_cast<double>(val)); } + + static constexpr double e = 2.718281828459045235360287471352662497757247093; + // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: prefer math constant [misc-math-constant] + ... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/66583 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits