[clang-tidy] add modernize-use-std-numbers #66583
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-clang-tidy ChangesThis check finds constants and function calls to math functions that can be replaced 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:
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]
|
|
For macros I have only implemented matching literals, not function calls or formulas. I also don't suggest using modern language constructs for macros, there are other checks that do this. |
5chmidti
force-pushed
the
clang_tidy_math_constant
branch
from
8fb9b11
to
6e14980
Compare
|
In general scope of the check is fine, but specially as it's limited to C++20 and way how its written would be good to rename it into modernize-use-std-numbers. |
|
Sure thing. I don't have a strong feeling about the category. |
5chmidti
force-pushed
the
clang_tidy_math_constant
branch
2 times, most recently
from
32d56e1
to
248c433
Compare
5chmidti
changed the title
I'ts not only about category but also name, check suggests std::numbers, then better name it modernize-use-std-numbers, reason for that is simple. Before C++20 we may want to have a check for example -prefer-math-constants that would change 3.14 hardcoded in code into M_PI. And in same way we could have check -use-std-numeric-limits that would change -2147483648 into std::numeric_limitsstd::sint32_t::min(). Simply we may have many of such checks. |
|
Simply better to reduce scope of check name to avoid overlap, unless check would support pre-c++20, then it could be fine. |
Sorry, I read too fast, it seems. But no objections to the name change on my end. It is more precise. I don't plan to implement pre c++20 macros or limits here. |
5chmidti
force-pushed
the
clang_tidy_math_constant
branch
from
248c433
to
0f14b63
Compare
5chmidti
changed the title
5chmidti
force-pushed
the
clang_tidy_math_constant
branch
2 times, most recently
from
26fb41a
to
8f5e9e6
Compare
clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-numbers.rst
Outdated
Show resolved
Hide resolved
clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-numbers.rst
Outdated
Show resolved
Hide resolved
Ping. |
I think you can't, instead of handling macro definitions, you just should handle places where macro is used. Instead of using MacroDefined and try to find those #define MY_PI, you should just use MacroExpands, and find usage of those system macros like M_PI, and suggest replacement usage of them without checking an value. As for #define MY_PI 3.14, this should be handled like it is now, simply isMacro shouldn't be used, and warning should be produced inside macro. |
Sounds good to do it like this for macros. My original thought about this was to be the as least intrusive as possible in terms of the number of matches and fixes this check does, but I'm all for replacing these macros. static constexpr double Phi = 1.6180339;
sink(Phi);be changed from static constexpr double Phi = std::numbers::phi;
sink(Phi)to static constexpr double Phi = std::numbers::phi;
sink(std::numbers::phi)? I could add an option
defaulting to... |
|
No need to change to |
5chmidti
force-pushed
the
clang_tidy_math_constant
branch
from
408b582
to
e4627bb
Compare
|
I rebased and force-pushed my branch to fix the formatting CI (fetch depth for comparison with head was too big). I'm not sure what the issue on Windows is. It looks like nothing within the function template |
|
Nice, the rebase fixed the Windows failure |
PiotrZSL
reviewed
Nov 3, 2023
PiotrZSL
reviewed
Nov 3, 2023
5chmidti
force-pushed
the
clang_tidy_math_constant
branch
from
60f4d26
to
284fa1b
Compare
|
I fetched from the wrong remote. Conflicts are resolved. The options docs were added in 284fa1b |
PiotrZSL
approved these changes
Dec 5, 2023
There was a problem hiding this comment.
Unless you want this to be merged as 44101708+5chmidti@users.noreply.github.com, correct your github settings, squash all commits into one and make sure that author of commit is correct.
clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-numbers.rst
Outdated
Show resolved
Hide resolved
clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-numbers.rst
Outdated
Show resolved
Hide resolved
|
That email is fine with me |
EugeneZelenko
requested changes
Dec 5, 2023
clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-numbers.rst
Outdated
Show resolved
Hide resolved
clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-numbers.rst
Outdated
Show resolved
Hide resolved
clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-numbers.rst
Outdated
Show resolved
Hide resolved
clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-numbers.rst
Outdated
Show resolved
Hide resolved
|
Thank you for the reviews |
|
@PiotrZSL check out https://lab.llvm.org/buildbot/#/builders/230/builds/22226, the bot was red on previous runs, but those are unrelated to how it fails now. The bot fails for this patch with the following build failure: The mismatch happens for the template arguments of llvm-project/clang-tools-extra/clang-tidy/modernize/UseStdNumbersCheck.cpp Lines 307 to 315 in 65cb5d5 should be declared static const auto ConstantMatchers = std::array<Matcher<Stmt>, 13>{
Matches.matchLog2Euler(), Matches.matchLog10Euler(),
Matches.matchEulerTopLevel(), Matches.matchEgamma(),
Matches.matchInvSqrtPi(), Matches.matchInvPi(),
Matches.matchPiTopLevel(), Matches.matchLn2(),
Matches.machterLn10(), Matches.matchSqrt2(),
Matches.matchInvSqrt3(), Matches.matchSqrt3(),
Matches.matchPhi(),
};FYI I don't have write access, so I cannot push a fix myself. Or just revert? |
PiotrZSL
added a commit
that referenced
this pull request
Dec 6, 2023
…ck.cpp Attempt to fix issue introduced in #66583
PiotrZSL
added a commit
that referenced
this pull request
Dec 6, 2023
Compilation issue, to be resolved. This reverts commit 3f73fd7.
|
I will compile this locally and try to fix it & recommit. |
PiotrZSL
pushed a commit
that referenced
this pull request
Dec 6, 2023
Finds constants and function calls to math functions that can be replaced with c++20's mathematical constants from the 'numbers' header and offers fix-it hints. Does not match the use of variables with that value, and instead, offers a replacement at the definition of those variables.
PiotrZSL
added a commit
that referenced
this pull request
Dec 6, 2023
…td-numbers To avoid compiler errors on some platforms introduced by #66583, now using std::vector to pass list of matchers into main matcher, and removed static variable as it could introduce some other issues.
|
Thanks |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Finds constants and function calls to math functions that can be replaced
with c++20's mathematical constants from the 'numbers' header and
offers fix-it hints.
Does not match the use of variables with that value, and instead,
offers a replacement at the definition of those variables.