Page MenuHomePhabricator

[clang-tidy] Add new check for math constants
Needs ReviewPublic

Authored by ZaMaZaN4iK on Aug 7 2019, 5:27 PM.

Details

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.

Diff Detail

Event Timeline

ZaMaZaN4iK created this revision.Aug 7 2019, 5:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 7 2019, 5:27 PM
ZaMaZaN4iK updated this revision to Diff 214043.Aug 7 2019, 5:38 PM

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.

Eugene.Zelenko retitled this revision from Add new check for math constants to [clang-tidy] Add new check for math constants.Aug 7 2019, 5:46 PM
Eugene.Zelenko added reviewers: alexfh, hokein.
Eugene.Zelenko set the repository for this revision to rG LLVM Github Monorepo.
aaron.ballman added inline comments.Aug 8 2019, 1:41 PM
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.

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?

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.

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/

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.

@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 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.

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?

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.

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

Can you share this script please? :)

... 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.

... 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

lebedev.ri resigned from this revision.Oct 16 2019, 11:12 AM
Szelethus resigned from this revision.Wed, Oct 30, 8:04 AM

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