[clang-tidy] Add new check for math constants

[ZaMaZaN4iK]

Summary

Hello.

I found quite interesting and useful FMPOV check for Clang Tidy - description, examples. Whole sense is in using standard (from cmath or math.h or math headers) math constants instead of written from scratch.

This patch is in status WIP. The main question is - do we want to see such check in Clang Tidy and should I continue work on it?

Thank you.

[Eugene.Zelenko]

May be this check belongs to readability module?

clang-tools-extra/clang-tidy/misc/MathConstantsCheck.cpp

13

Unnecessary empty line.

27

Functions and constants should be static. See LLVM Coding Guidelines.

70

Is it needed?

84

Please don't use auto when type is not spelled in same statement or in case of iterators. Same in other places.

85

Unnecessary empty line.

clang-tools-extra/clang-tidy/misc/MathConstantsCheck.h

13

Please remove empty lines between headers.

clang-tools-extra/docs/ReleaseNotes.rst

91

Please add one sentence description. Should be same as first sentence of documentation.

clang-tools-extra/docs/clang-tidy/checks/misc-math-constants.rst

6

Please add documentation.

[aaron.ballman]

clang-tools-extra/clang-tidy/misc/MathConstantsCheck.cpp

44–58

I'm a bit uncomfortable putting the constant values in like this -- are you sure those values will be correct for all targets?

clang-tools-extra/clang-tidy/misc/MathConstantsCheck.h

26

You should fix this FIXME. ;-)

42

Formatting is off here, you should run the patch through clang-format, if you don't already.

[ZaMaZaN4iK]

The main reason why I've created this differential - asking to you about usefulness of this check for clang-tidy. I understand that there are a some TODO and formatting issues - it's ok for now.

As far I understand your remarks - you are not against this check so I can continue my work on this path. Great.

Let's discuss about some open questions:

1. Should we support C language and <math.h> header file? I think - yes, it's quite easy to implement.
2. Should we support converting to Boost.Constants? I don't think so.
3. In header file instead of defining math constants directly should we use them from <cmath> as much as possible?

[aaron.ballman]

In D65912#1621898, @ZaMaZaN4iK wrote:

The main reason why I've created this differential - asking to you about usefulness of this check for clang-tidy. I understand that there are a some TODO and formatting issues - it's ok for now.

As far I understand your remarks - you are not against this check so I can continue my work on this path. Great.

I think the idea is a reasonable one, yes. One thing I would be interested in knowing is how often the check behaves when run over some large, real-world code bases. Does it catch any true positives? Does it have false positives?

Let's discuss about some open questions:

1. Should we support C language and <math.h> header file? I think - yes, it's quite easy to implement.

I think so, yes.

1. Should we support converting to Boost.Constants? I don't think so.

Perhaps as a follow-up patch, if someone was interested in doing the work.

1. In header file instead of defining math constants directly should we use them from <cmath> as much as possible?

Yes, but you should probably ignore system header files so that you don't flag implementations providing their own sets of constants.

[ZaMaZaN4iK]

One thing I would be interested in knowing is how often the check behaves when run over some large, real-world code bases. Does it catch any true positives? Does it have false positives?

I have no such statistics for this check. But I have statistics for identical check from PVS-Studio - https://www.viva64.com/en/examples/v624/

[aaron.ballman]

In D65912#1622963, @ZaMaZaN4iK wrote:

One thing I would be interested in knowing is how often the check behaves when run over some large, real-world code bases. Does it catch any true positives? Does it have false positives?

I have no such statistics for this check. But I have statistics for identical check from PVS-Studio - https://www.viva64.com/en/examples/v624/

If you're trying to match that check's behavior, it may change how we proceed on this patch -- for instance, PVS seems to catch constants similar to known constants but are not exact matches. You kind of do that, but we should probably make the epsilon configurable and have some data behind whatever default value we pick in terms of false positives triggered.

[ZaMaZaN4iK]

@aaron.ballman Sure. I agree with you that epsilon should be configurable. I think we can collect some statistics later. Now I am going to work on implementation and tests. Later, of course, will be good to run the check on some codebases. I will be happy to hear that we already have some infrastructure for doing stuff like this.

[aaron.ballman]

In D65912#1623076, @ZaMaZaN4iK wrote:

@aaron.ballman Sure. I agree with you that epsilon should be configurable. I think we can collect some statistics later. Now I am going to work on implementation and tests. Later, of course, will be good to run the check on some codebases. I will be happy to hear that we already have some infrastructure for doing stuff like this.

My point regarding statistics is that the check needs to pull its own weight -- if it doesn't find many true positives, it's not of much value to a broad community, or if it has a lot of false positives, we may need to tweak the check before releasing it to the public, etc. So definitely do the implementation work, but part of that work should be testing it over large code bases and reporting back the results.

[ZaMaZaN4iK]

My point regarding statistics is that the check needs to pull its own weight -- if it doesn't find many true positives, it's not of much value to a broad community, or if it has a lot of false positives, we may need to tweak the check before releasing it to the public, etc. So definitely do the implementation work, but part of that work should be testing it over large code bases and reporting back the results.

Okay. Do we have any infrastructure for doing such testing? Or I should do it manually: prepare some large codebases, run over them clang-tidy with the check and parse the result?

[aaron.ballman]

In D65912#1623293, @ZaMaZaN4iK wrote:

My point regarding statistics is that the check needs to pull its own weight -- if it doesn't find many true positives, it's not of much value to a broad community, or if it has a lot of false positives, we may need to tweak the check before releasing it to the public, etc. So definitely do the implementation work, but part of that work should be testing it over large code bases and reporting back the results.

Okay. Do we have any infrastructure for doing such testing? Or I should do it manually: prepare some large codebases, run over them clang-tidy with the check and parse the result?

We don't really have an automated way to do this (that I am aware of, anyway). I typically find large code bases that either use CMake directly or can use bear so that I can generate a compile_commands.json file, then I have a script that runs clang-tidy over all the compilation units in a compilation database.

[ZaMaZaN4iK]

... then I have a script that runs clang-tidy over all the compilation units in a compilation database

Can you share this script please? :)

[Eugene.Zelenko]

In D65912#1623375, @ZaMaZaN4iK wrote:

... then I have a script that runs clang-tidy over all the compilation units in a compilation database

Can you share this script please? :)

run-clang-tidy.py which is shipped with Clang-tidy.

[aaron.ballman]

In D65912#1623393, @Eugene.Zelenko wrote:

In D65912#1623375, @ZaMaZaN4iK wrote:

... then I have a script that runs clang-tidy over all the compilation units in a compilation database

Can you share this script please? :)

run-clang-tidy.py which is shipped with Clang-tidy.

Hah! Looks like I don't need my local script any longer. Thank you for reminding me that this exists. :-D

[Szelethus]

I have no authority in clang-tidy, but the idea is nice! You may wanna reupload with full context though.

[JonasToth]

In D65912#1623375, @ZaMaZaN4iK wrote:

... then I have a script that runs clang-tidy over all the compilation units in a compilation database

Can you share this script please? :)

I did a bit of work on infrastructure for testing at some point, but never really finished something that works great and nice, just some script-ideas you can utilize.

You can use https://reviews.llvm.org/D54141 to deduplicate messages, especially such constants might be declared in headers that are included often.
Once you deduped the diagnostics you can pipe the output of run-clang-tidy.py in a file and do simple line counts and scroll though to get an idea of your checks output.

clang-tools-extra/clang-tidy/misc/CMakeLists.txt

 
 add_clang_library(clangTidyMiscModule
   DefinitionsInHeadersCheck.cpp
+  MathConstantsCheck.cpp
   MiscTidyModule.cpp
   MisplacedConstCheck.cpp
   NewDeleteOverloadsCheck.cpp

clang-tools-extra/clang-tidy/misc/MathConstantsCheck.h

+//===--- MathConstantsCheck.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_MATHCONSTANTSCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MATHCONSTANTSCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+#include "../utils/IncludeInserter.h"
+
+#include "llvm/ADT/Optional.h"
+
+#include <memory>
+#include <string>
+#include <vector>
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+/// FIXME: Write a short description.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-math-constants.html
+class MathConstantsCheck : public ClangTidyCheck {
+public:
+  MathConstantsCheck(StringRef Name, ClangTidyContext *Context);
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
+                           Preprocessor *ModuleExpanderPP) override;
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+
+private:
+  void insertHeader(DiagnosticBuilder &Diag, FileID FD);
+
+  struct MathConstantDescription
+  {
+      const Optional<std::string> NameInC;
+      const Optional<std::string> NameInCpp20;
+      const double Value;
+  };
+
+  const std::vector<MathConstantDescription> Constants;
+  const utils::IncludeSorter::IncludeStyle IncludeStyle;
+  std::string ChosenHeaderName;
+  std::unique_ptr<utils::IncludeInserter> Inserter;
+};
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MATHCONSTANTSCHECK_H

clang-tools-extra/clang-tidy/misc/MathConstantsCheck.cpp

+//===--- MathConstantsCheck.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 "MathConstantsCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "../../../clang/unittests/AST/Language.h"
+
+#include <cmath>
+#include <iostream>
+#include <string>
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+namespace
+{
+
+constexpr char StdMathHeader[] = "math";
+constexpr char StdCmathHeader[] = "cmath";
+constexpr char StdCHeader[] = "math.h";
+constexpr char FloatType[] = "floatLiteral";
+constexpr double Epsilon = 0.01;
+
+std::string GetAngledName(const std::string& filename)
+{
+    return "<" + filename + ">";
+}
+
+} // namespace
+
+MathConstantsCheck::MathConstantsCheck(StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+    Constants
+    {
+            {{"M_E"}, {"std::math::e"}, 2.7182818284590452354},
+            {{"M_LOG2E"}, {"std::math::log2e"}, 1.4426950408889634074},
+            {{"M_LOG10E"}, {"std::math::log10e"}, 0.43429448190325182765},
+            {{"M_LN2"}, {"std::math::ln2"}, 0.69314718055994530942},
+            {{"M_LN10"}, {"std::math::ln10"}, 2.30258509299404568402},
+            {{"M_PI"}, {"std::math::pi"}, 3.14159265358979323846},
+            {{"M_PI_2"}, {}, 1.57079632679489661923},
+            {{"M_PI_4"}, {}, 0.78539816339744830962},
+            {{"M_1_PI"}, {"std::math::inv_pi"}, 0.31830988618379067154},
+            {{"M_2_PI"}, {}, 0.63661977236758134308},
+            {{"M_2_SQRTPI"}, {"std::math::inv_sqrtpi"}, 1.12837916709551257390},
+            {{"M_SQRT2"}, {"std::math::sqrt2"}, 1.41421356237309504880},
+            {{"M_SQRT1_2"}, {"std::math::"}, 0.70710678118654752440},
+            {{}, {"std::math::phi"}, 1.6180339887498948482},
+            {{}, {"std::math::egamma"}, 0.5772156649015328606 }
+    },
+    IncludeStyle(utils::IncludeSorter::parseIncludeStyle(
+              Options.getLocalOrGlobal("IncludeStyle", "llvm")))
+{
+}
+
+void MathConstantsCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+    Options.store(Opts, "IncludeStyle", IncludeStyle);
+}
+
+void MathConstantsCheck::registerMatchers(MatchFinder *Finder) {
+  // FIXME: Add matchers.
+  Finder->addMatcher(floatLiteral().bind(FloatType), this);
+}
+
+void MathConstantsCheck::registerPPCallbacks(const SourceManager &SM,
+                                             Preprocessor *PP,
+                                             Preprocessor *ModuleExpanderPP) {
+        Inserter = llvm::make_unique<utils::IncludeInserter>(SM, getLangOpts(),
+                                                             IncludeStyle);
+        PP->addPPCallbacks(Inserter->CreatePPCallbacks());
+}
+
+void MathConstantsCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *MatchedDecl = Result.Nodes.getNodeAs<FloatingLiteral>(FloatType);
+  const auto MatchedFloatConstant = MatchedDecl->getValue().convertToDouble();
+
+  const auto Language = getLangOpts();
+
+  for (const auto& MathConstant : Constants)
+  {
+      // Match constant
+      if (std::fabs(MatchedFloatConstant - MathConstant.Value) < Epsilon)
+      {
+          std::string ConstantName;
+          if(getLangOpts().CPlusPlus2a)
+          {
+              if(MathConstant.NameInCpp20.hasValue())
+              {
+                  ConstantName = MathConstant.NameInCpp20.getValue();
+                  ChosenHeaderName = StdMathHeader;
+              }
+              else
+              {
+                  ConstantName = MathConstant.NameInC.getValue();
+                  ChosenHeaderName = StdCmathHeader;
+              }
+          }
+          else // For other C++ standards and whole C
+          {
+              if(!MathConstant.NameInC.hasValue())
+              {
+                  // We have not such constant for old standards
+                  return;
+              }
+
+              ConstantName = MathConstant.NameInC.getValue();
+              if (Language.C99 || Language.C11 || Language.C17 || Language.C2x)
+              {
+                  ChosenHeaderName = StdCHeader;
+              }
+              else if (Language.CPlusPlus)
+              {
+                  ChosenHeaderName = StdCmathHeader;
+              }
+          }
+
+          diag(MatchedDecl->getLocation(), "The constant " + std::to_string(MatchedFloatConstant) + " is being utilized. "
+            "The resulting value could be inaccurate. Consider using the " + ConstantName +
+            " constant from " + GetAngledName(ChosenHeaderName));
+
+          // Fix-hint
+          auto Diag = diag(MatchedDecl->getLocation(), "insert '" + ConstantName + "'", DiagnosticIDs::Note)
+                  << FixItHint::CreateReplacement(MatchedDecl->getLocation(), ConstantName);
+
+          SourceManager &SM = *Result.SourceManager;
+          insertHeader(Diag, SM.getFileID(MatchedDecl->getExprLoc()));
+      }
+  }
+}
+
+void MathConstantsCheck::insertHeader(DiagnosticBuilder &Diag, FileID FD) {
+    if (ChosenHeaderName.empty()) {
+        return;
+    }
+    if (auto IncludeFixit = Inserter->CreateIncludeInsertion(
+            FD, ChosenHeaderName,
+            true)) {
+        Diag << *IncludeFixit;
+    }
+}
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang

clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp

 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "DefinitionsInHeadersCheck.h"
+#include "MathConstantsCheck.h"
 #include "MisplacedConstCheck.h"
 #include "NewDeleteOverloadsCheck.h"
 #include "NonCopyableObjects.h"

   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<DefinitionsInHeadersCheck>(
         "misc-definitions-in-headers");
+    CheckFactories.registerCheck<MathConstantsCheck>(
+        "misc-math-constants");
     CheckFactories.registerCheck<MisplacedConstCheck>("misc-misplaced-const");
     CheckFactories.registerCheck<NewDeleteOverloadsCheck>(
         "misc-new-delete-overloads");

clang-tools-extra/docs/ReleaseNotes.rst

 
 The improvements are...
 
+- New :doc:`misc-math-constants
+  <clang-tidy/checks/misc-math-constants>` check.
+
+  FIXME: add release notes.
+
 Improvements to clang-include-fixer
 -----------------------------------
 

clang-tools-extra/docs/clang-tidy/checks/list.rst

    llvm-prefer-isa-or-dyn-cast-in-conditionals
    llvm-twine-local
    misc-definitions-in-headers
+   misc-math-constants
    misc-misplaced-const
    misc-new-delete-overloads
    misc-non-copyable-objects

clang-tools-extra/docs/clang-tidy/checks/misc-math-constants.rst

+.. title:: clang-tidy - misc-math-constants
+
+misc-math-constants
+===================
+
+FIXME: Describe what patterns does the check detect and why. Give examples.

clang-tools-extra/test/clang-tidy/misc-math-constants.cpp

+// RUN: %check_clang_tidy %s misc-math-constants %t
+
+// FIXME: Add something that triggers the check here.
+void f();
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function 'f' is insufficiently awesome [misc-math-constants]
+
+// FIXME: Verify the applied fix.
+//   * Make the CHECK patterns specific enough and try to make verified lines
+//     unique to avoid incorrect matches.
+//   * Use {{}} for regular expressions.
+// CHECK-FIXES: {{^}}void awesome_f();{{$}}
+
+// FIXME: Add something that doesn't trigger the check here.
+void foo()
+{
+    double pi = 3.14;
+    double p_2 = 1.57;
+}