[Clang-tidy] readability check to convert numerical constants to std::numeric_limits

[IdrissRio]

Summary

Hello, i want to propose this check suggested by @EugeneZelenko.
I have found it in the beginner tag of llvm-bugzilla repository.

This check looks for numerical unsigned constants that are equal to -1 or ~0
and substitutes them with std::numeric_limits<type>::max().

It includes the library <limits> if is not found.

Example:

unsigned const int x = -1;

becomes

unsigned const int x = std::numeric_limits<unsigned int>::max();

If the library <limits> is not include in the TU the check will add it with this criteria:

• If a #include is found it will add #include <limits> before this include.
• if no #include directive is found it will add it at the begin of the TU.

[lebedev.ri]

The check itself isn't in the diff.

[IdrissRio]

Sorry i have forgot to add the source code.

[JonasToth]

Hi IdrissRio,

thanks for working on this! I have one question: Why are variables _not_ considered in the check but only constants? IMHO it would make sense to transform these as well.

I dont have time for long review today anymore, I will continue tomorrow.

clang-tidy/readability/NumericalCostantsToMaxIntCheck.cpp

22 ↗

(On Diff #168321)

Unfortunatly this is duplicated effort.
Please take a look at cppcoreguidelines/ProBoundsConstantArrayIndexCheck.cpp and
utils/IncludeInserter.{h,cpp} to add new includes from clang-tidy

90 ↗

(On Diff #168321)

Is failure here expected? The matcher should match both nodes simultanously, if so please use an assert instead to detect unexpected failures

docs/clang-tidy/checks/readability-numerical-costants-to-max-int.rst

16 ↗

(On Diff #168321)

please indent that code, otherwise its is rendered incorrect

[Eugene.Zelenko]

I think modernize is better module for this check.

clang-tidy/readability/NumericalCostantsToMaxIntCheck.cpp

58 ↗

(On Diff #168321)

Semicolon at end is not necessary. Please also run Clang-format.

87 ↗

(On Diff #168321)

Unnecessary empty line.

docs/ReleaseNotes.rst

60

Please use alphabetical order for new checks.

63

Please enclose -1 and ~0 in `.

64

Please enclose std::numeric_limits<type>::max() in ``.

docs/clang-tidy/checks/readability-numerical-costants-to-max-int.rst

7 ↗

(On Diff #168321)

Please synchronize with Release Notes.

[Eugene.Zelenko]

It'll be good idea to handle climits constants too.

[jfb]

Can you also catch casts such as (unsigned)-1 ?

[Eugene.Zelenko]

clang-tidy/readability/NumericalCostantsToMaxIntCheck.cpp

44 ↗

(On Diff #168321)

Unnecessary empty line.

[JonasToth]

@aaron.ballman I have a question for an expert: How would bitfields relate to this check? Can there be a similar pattern for them and do they need to be handled here?

clang-tidy/readability/NumericalCostantsToMaxIntCheck.cpp

62 ↗

(On Diff #168321)

you dont need this-> and please use the same return early pattern as in the other registering call

64 ↗

(On Diff #168321)

its uncommong to specify the global namespace (first ::), you can remove these

77 ↗

(On Diff #168321)

Please use the full name for the bind, Literal or IntLiteral (please consistent with VarDecl, so CamelCase). I feel that this is more readable (no strong opinion though if you want to leave it as is)

93 ↗

(On Diff #168321)

Please declar this variable later, directly before usage (locality), and add const SourceManager* SM as you don't seem to manipulate SM

95 ↗

(On Diff #168321)

You can use llvm::Twine here which is very efficient for concatenation.

std::string Replacement = (Twine("std::numeric_limits<") + Decl->getType().getAtomicUnqualifiedType().getAsString() + ">::max()").str();

107 ↗

(On Diff #168321)

I think its better to create the source-range before to deal with exceptional cases (invalid ranges, macros)

docs/clang-tidy/checks/list.rst

12

spurious change, does it still happen on master? i thought this was ordered at some point already

219

this does not follow the naming convention (hyphens and not CamelCase)

docs/clang-tidy/checks/readability-numerical-costants-to-max-int.rst

4 ↗

(On Diff #168321)

please make === full length.

10 ↗

(On Diff #168321)

This line is slightly inaccurate. The library is the standard library, s/library/header-file

37 ↗

(On Diff #168321)

Please remove the empty lines at the end

test/clang-tidy/readability-numerical-costants-to-max-int.cpp

9 ↗

(On Diff #168321)

Please add testcases for macros (literal in macro, variable declaration in macro, other possibilities you might find) and for templates.

template <typename Foo>
void f() {
    Foo f = -1;
}

template <int NonType>
void f2() {
    decltype(NonType) new_var = -1;
}

and variations. Please try to cover as many possible constructs as you can.

Another interesting case are enums, example:

enum FooEnum : unsigned {
   bar = 0,
   foobar = 1,
   maximum = -1
}

You don't need to handle this case yet, but please add a testcase to demonstrate the behaviour of the check for it. Adding a TODO: would be perfect to signal future extendability.

[Eugene.Zelenko]

clang-tidy/readability/NumericalCostantsToMaxIntCheck.cpp

62 ↗

(On Diff #168321)

By the word, see PR32774 for such check suggestion.

[IdrissRio]

Update with the suggestion proposed by @Eugene.Zelenko and @JonasToth

[lebedev.ri]

clang-tidy/readability/NumericalCostantsToMaxIntCheck.h

25 ↗

(On Diff #168662)

I feel like the name is overly vague.
This *only* handles the cases of -1 and ~0.
It does not handle cases like 255 -> std::numeric_limits<uint_8>::max().
(It might be nice to do that, but it is more complex i suspect.)

[Eugene.Zelenko]

docs/ReleaseNotes.rst

118

Wrong kind of quotes were used. Should be . std::numeric_limits<type>::max() Should be enclosed in `.

docs/clang-tidy/checks/readability-numerical-costants-to-max-int.rst

7 ↗

(On Diff #168662)

Please re-synchronize.

[JonasToth]

clang-tidy/readability/NumericalCostantsToMaxIntCheck.cpp

63 ↗

(On Diff #168662)

please assert Lit as well and add an error message in the assert, like assert(Decl && Lit && "Expect both matchers to match");

docs/clang-tidy/checks/readability-numerical-costants-to-max-int.rst

45 ↗

(On Diff #168662)

Please remove the empty line at the end

test/clang-tidy/readability-numerical-costants-to-max-int.cpp

72 ↗

(On Diff #168662)

Please add tests that use typedefs (and using = ...) to mask the underlying type. If they dont work, use hasCanonicalType in the matcher, as the canonical type looks through the typedefs.

Common typedefs are uint32_t and the like to control the bitwidth of the integer.

[aaron.ballman]

I like the thrust of this check, but we already have the readability-magic-numbers check and I think that this functionality would fit naturally there. That check finds numerical constants and recommends turning them into named constants, but for special values it seems reasonable to recommend using numeric_limits instead. What do others think of that, from an organizational perspective?

[JonasToth]

In D52892#1263324, @aaron.ballman wrote:

I like the thrust of this check, but we already have the readability-magic-numbers check and I think that this functionality would fit naturally there. That check finds numerical constants and recommends turning them into named constants, but for special values it seems reasonable to recommend using numeric_limits instead. What do others think of that, from an organizational perspective?

+1, totally forgot that one. I feel that the bugprone is more strict though, so adding an readability-mode for it that only warns for the numeric_limits constants seems desirable.

clang-tidy/readability/CMakeLists.txt

add_clang_library(clangTidyReadabilityModule
   ImplicitBoolConversionCheck.cpp
   InconsistentDeclarationParameterNameCheck.cpp
   MagicNumbersCheck.cpp
   MisleadingIndentationCheck.cpp
   MisplacedArrayIndexCheck.cpp
   NamedParameterCheck.cpp
   NamespaceCommentCheck.cpp
   NonConstParameterCheck.cpp
+  NumericalCostantsToMaxIntCheck.cpp
   ReadabilityTidyModule.cpp
   RedundantControlFlowCheck.cpp
   RedundantDeclarationCheck.cpp
   RedundantFunctionPtrDereferenceCheck.cpp
   RedundantMemberInitCheck.cpp
   RedundantSmartptrGetCheck.cpp
   RedundantStringCStrCheck.cpp
   RedundantStringInitCheck.cpp

clang-tidy/readability/ReadabilityTidyModule.cpp

 #include "IdentifierNamingCheck.h"
 #include "ImplicitBoolConversionCheck.h"
 #include "InconsistentDeclarationParameterNameCheck.h"
 #include "MagicNumbersCheck.h"
 #include "MisleadingIndentationCheck.h"
 #include "MisplacedArrayIndexCheck.h"
 #include "NamedParameterCheck.h"
 #include "NonConstParameterCheck.h"
+#include "NumericalCostantsToMaxIntCheck.h"
 #include "RedundantControlFlowCheck.h"
 #include "RedundantDeclarationCheck.h"
 #include "RedundantFunctionPtrDereferenceCheck.h"
 #include "RedundantMemberInitCheck.h"
 #include "RedundantSmartptrGetCheck.h"
 #include "RedundantStringCStrCheck.h"
 #include "RedundantStringInitCheck.h"
 #include "SimplifyBooleanExprCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<InconsistentDeclarationParameterNameCheck>(
         "readability-inconsistent-declaration-parameter-name");
     CheckFactories.registerCheck<MagicNumbersCheck>(
         "readability-magic-numbers");
     CheckFactories.registerCheck<MisleadingIndentationCheck>(
         "readability-misleading-indentation");
     CheckFactories.registerCheck<MisplacedArrayIndexCheck>(
         "readability-misplaced-array-index");
+    CheckFactories.registerCheck<NumericalCostantsToMaxIntCheck>(
+        "readability-numerical-costants-to-max-int");
     CheckFactories.registerCheck<RedundantFunctionPtrDereferenceCheck>(
         "readability-redundant-function-ptr-dereference");
     CheckFactories.registerCheck<RedundantMemberInitCheck>(
         "readability-redundant-member-init");
     CheckFactories.registerCheck<SimplifySubscriptExprCheck>(
         "readability-simplify-subscript-expr");
     CheckFactories.registerCheck<StaticAccessedThroughInstanceCheck>(
         "readability-static-accessed-through-instance");

docs/ReleaseNotes.rst

 Improvements to clang-rename
 ----------------------------
 
 The improvements are...
 
 Improvements to clang-tidy
 --------------------------
 
+- New :doc:`readability-numerical-costants-to-max-int
+  <clang-tidy/checks/readability-numerical-costants-to-max-int>` check.
+
+  Checks for numerical unsigned constants that are equal to -1 or ~0
+  and substitutes them with std::numeric_limits<type>::max().
+
 - New :doc:`abseil-duration-division
   <clang-tidy/checks/abseil-duration-division>` check.
 
   Checks for uses of ``absl::Duration`` division that is done in a
   floating-point context, and recommends the use of a function that
   returns a floating-point value.
 
 - New :doc:`abseil-faster-strsplit-delimiter
- New :doc:`readability-magic-numbers
   <clang-tidy/checks/readability-magic-numbers>` check.
 
   Detects usage of magic numbers, numbers that are used as literals instead of
   introduced via constants or symbols.
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 
 Improvements to modularize
 --------------------------
 
 The improvements are...

docs/clang-tidy/checks/list.rst

 .. title:: clang-tidy - Clang-Tidy Checks
 
 Clang-Tidy Checks
 =================
 
 .. toctree::
    abseil-duration-division
    abseil-faster-strsplit-delimiter
    abseil-no-internal-dependencies
    abseil-no-namespace
    abseil-redundant-strcat-calls
-   abseil-string-find-startswith
    abseil-str-cat-append
+   abseil-string-find-startswith
    android-cloexec-accept
    android-cloexec-accept4
    android-cloexec-creat
    android-cloexec-dup
    android-cloexec-epoll-create
    android-cloexec-epoll-create1
    android-cloexec-fopen
    android-cloexec-inotify-init
.. toctree::
    performance-inefficient-vector-operation
    performance-move-const-arg
    performance-move-constructor-init
    performance-noexcept-move-constructor
    performance-type-promotion-in-math-fn
    performance-unnecessary-copy-initialization
    performance-unnecessary-value-param
    portability-simd-intrinsics
+   readability-NumericalCostantsToMaxInt
    readability-avoid-const-params-in-decls
    readability-braces-around-statements
    readability-container-size-empty
    readability-delete-null-pointer
    readability-deleted-default
    readability-else-after-return
    readability-function-size
    readability-identifier-naming