[PATCH] D65912: [clang-tidy] Add new check for math constants

JonasToth added a comment. Herald added a subscriber: whisperity. 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?

[PATCH] D65912: [clang-tidy] Add new check for math constants

Szelethus resigned from this revision. Szelethus added a comment. I have no authority in clang-tidy, but the idea is nice! You may wanna reupload with full context though. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65912/new/

[PATCH] D65912: [clang-tidy] Add new check for math constants

aaron.ballman added a comment. 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

[PATCH] D65912: [clang-tidy] Add new check for math constants

Eugene.Zelenko added a comment. 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

[PATCH] D65912: [clang-tidy] Add new check for math constants

ZaMaZaN4iK added a comment. > ... then I have a script that runs clang-tidy over all the compilation units > in a compilation database Can you share this script please? :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65912/new/

[PATCH] D65912: [clang-tidy] Add new check for math constants

aaron.ballman added a comment. 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

[PATCH] D65912: [clang-tidy] Add new check for math constants

ZaMaZaN4iK added a comment. > 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

[PATCH] D65912: [clang-tidy] Add new check for math constants

aaron.ballman added a comment. 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.

[PATCH] D65912: [clang-tidy] Add new check for math constants

ZaMaZaN4iK added a comment. @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

[PATCH] D65912: [clang-tidy] Add new check for math constants

aaron.ballman added a comment. 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

[PATCH] D65912: [clang-tidy] Add new check for math constants

ZaMaZaN4iK added a comment. > 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

[PATCH] D65912: [clang-tidy] Add new check for math constants

aaron.ballman added a comment. 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 -

[PATCH] D65912: [clang-tidy] Add new check for math constants

ZaMaZaN4iK added a comment. 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

[PATCH] D65912: [clang-tidy] Add new check for math constants

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/misc/MathConstantsCheck.cpp:44-58 +{{"M_E"}, {"std::math::e"}, 2.7182818284590452354}, +{{"M_LOG2E"}, {"std::math::log2e"}, 1.4426950408889634074}, +{{"M_LOG10E"},