Page MenuHomePhabricator

[clang-tidy] Add misc-default-numerics
Needs RevisionPublic

Authored by Prazek on May 23 2017, 3:41 PM.

Details

Summary

This check flags usages of `std::numeric_limits<T>::{min,max}()` for
unspecialized types. It is dangerous because returns T(), which might is
rarely
minimum or maximum for this type.

Consider scenario:

  1. Have typedef long long BigInt in source code
  2. Use std::numeric_limits<BigInt>::min()
  3. Replace the BigInt typedef with class implementing BigIntegers
  4. Compile silently your code and find it few years later that you have

of by
9223372036854775808 error.

Event Timeline

Prazek created this revision.May 23 2017, 3:41 PM
Prazek added a project: Restricted Project.May 23 2017, 3:42 PM
Eugene.Zelenko added inline comments.
docs/clang-tidy/checks/misc-default-numerics.rst
12

May be code-block will be better?

Prazek added inline comments.May 24 2017, 5:11 AM
docs/clang-tidy/checks/misc-default-numerics.rst
12

Right, I was looking for something like an inline code block, but I guess I will just change it to code block with comments

Prazek updated this revision to Diff 100070.May 24 2017, 5:13 AM
  • fixed docs
aaron.ballman added inline comments.May 24 2017, 10:29 AM
clang-tidy/misc/DefaultNumericsCheck.cpp
38

Can remove the spurious newline.

42

This diagnostic doesn't tell the user what's wrong with their code. I think something along these lines might be better:

'std::numeric_limits::%select{max|min}0' called with type %1; no such specialization exists, so the default value for that type is returned

clang-tidy/misc/DefaultNumericsCheck.h
21

because returns -> because it returns

docs/clang-tidy/checks/misc-default-numerics.rst
8

because returns... -> because the calls return ''T()'' in this case, which is unlikely to represent the minimum or maximum value for the type.

20

Remove spurious newline

25

Is this meant to say "off by X error"? I think a better wording would be that the code continues to compile, but the call to min() returns BigInt{}, or something more explicit.

Prazek updated this revision to Diff 100216.May 25 2017, 2:40 AM
Prazek marked 8 inline comments as done.
  • Thanks for the review Aaron, that is much better.
aaron.ballman edited edge metadata.May 25 2017, 1:51 PM

Once you fix the typo in the check, can you run it over some large C++ code bases to see if it finds any results?

clang-tidy/misc/DefaultNumericsCheck.cpp
31

This should be checking for ::std::numeric_limits (plural).

docs/ReleaseNotes.rst
77

numeric_limits

test/clang-tidy/misc-default-numerics.cpp
7

numeric_limits (same elsewhere in this file).

alexshap added inline comments.
clang-tidy/misc/DefaultNumericsCheck.h
21

nit: (feel free to correct me)
replace
"which might is ..."
with
"which rarely might be minimum or maximum for this type"

Prazek updated this revision to Diff 103837.Jun 24 2017, 3:09 AM
Prazek marked 4 inline comments as done.
  • fixed docs
  • fixes

Once you fix the typo in the check, can you run it over some large C++ code bases to see if it finds any results?

I tried it on LLVM code base (after fixing bug with the numeric_limits name) and it didn't find anything suspisious.
Unfortunatelly I don't have enough time to try it on different codebases, but I am weiling to fix any bug with this check if it would happen in the future.
The release 5.0 is near, so I would like to push it upstream. Does it sound good to you?

Once you fix the typo in the check, can you run it over some large C++ code bases to see if it finds any results?

I tried it on LLVM code base (after fixing bug with the numeric_limits name) and it didn't find anything suspisious.
Unfortunatelly I don't have enough time to try it on different codebases, but I am weiling to fix any bug with this check if it would happen in the future.
The release 5.0 is near, so I would like to push it upstream. Does it sound good to you?

My concern is: does this find any actual issues in real world code? This seems like such a highly specific check -- not many people use numeric_limits in the first place, let alone on non-builtin types, so does it justify running this check when someone batch-includes all of the misc checks?

I don't think this check is going to trigger a ton of false positives. I am wondering more the opposite: will this check ever trigger on anything other than compiler test cases?

clang-tidy/misc/DefaultNumericsCheck.h
21

the minimum or maximum (add the "the").

test/clang-tidy/misc-default-numerics.cpp
32

Can you add a test case where numeric_limits has been properly specialized for the type and the type is not a builtin?

Once you fix the typo in the check, can you run it over some large C++ code bases to see if it finds any results?

I tried it on LLVM code base (after fixing bug with the numeric_limits name) and it didn't find anything suspisious.
Unfortunatelly I don't have enough time to try it on different codebases, but I am weiling to fix any bug with this check if it would happen in the future.
The release 5.0 is near, so I would like to push it upstream. Does it sound good to you?

My concern is: does this find any actual issues in real world code? This seems like such a highly specific check -- not many people use numeric_limits in the first place, let alone on non-builtin types, so does it justify running this check when someone batch-includes all of the misc checks?

I don't think this check is going to trigger a ton of false positives. I am wondering more the opposite: will this check ever trigger on anything other than compiler test cases?

The check is based on the real world scenario that my friend had at work, so at least I know about one such case :)
It probably won't be a very popular check in terms of fiding anything, but it should find real bugs. The matcher is very easy so the check should be fast.

Prazek updated this revision to Diff 104281.Jun 27 2017, 3:04 PM
Prazek marked 2 inline comments as done.
  • fixed docs
  • fixes
  • Last fixes?
Prazek updated this revision to Diff 104288.Jun 27 2017, 3:43 PM

Small fix

Prazek updated this revision to Diff 104297.Jun 27 2017, 3:53 PM

fixed broken test

aaron.ballman added inline comments.Jul 5 2017, 6:38 AM
test/clang-tidy/misc-default-numerics.cpp
29

This is not a proper specialization according to the standard.

A program may add a template specialization for any standard library template to namespace std only if the declaration depends on a user-defined type and the specialization meets the standard library requirements for the original template and is not explicitly prohibited.

The functions are not marked constexpr or noexcept, and min() must return SpecializedType. Also the is_specialized member needs to be set to true. There are other requirements missing as well, but I think fixing the function signatures is the only important one.

alexfh requested changes to this revision.Mar 14 2018, 8:19 AM

Should the check be in "bugprone-" instead?

This revision now requires changes to proceed.Mar 14 2018, 8:19 AM