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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

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



================
Comment at: clang-tidy/readability/NumericalCostantsToMaxIntCheck.cpp:62
+    CompilerInstance &Compiler) {
+  if (this->getLangOpts().CPlusPlus) {
+    Compiler.getPreprocessor().addPPCallbacks(
----------------
you dont need `this->` and please use the same return early pattern as in the other registering call


================
Comment at: clang-tidy/readability/NumericalCostantsToMaxIntCheck.cpp:64
+    Compiler.getPreprocessor().addPPCallbacks(
+        ::llvm::make_unique<NumericalConstCheckPPCallbacks>(InsertLimits,
+                                                            &IncludeLocation));
----------------
its uncommong to specify the global namespace (first `::`), you can remove these


================
Comment at: clang-tidy/readability/NumericalCostantsToMaxIntCheck.cpp:77
+                    hasOperatorName("-"),
+                    hasUnaryOperand(integerLiteral(equals(1)).bind("Lit"))))),
+                hasInitializer(ignoringImpCasts(unaryOperator(
----------------
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)


================
Comment at: clang-tidy/readability/NumericalCostantsToMaxIntCheck.cpp:93
+
+  SourceManager *SM = Result.SourceManager;
+  std::string InsteadOf = "-1";
----------------
Please declar this variable later, directly before usage (locality), and add `const SourceManager* SM` as you don't seem to manipulate `SM`


================
Comment at: clang-tidy/readability/NumericalCostantsToMaxIntCheck.cpp:95
+  std::string InsteadOf = "-1";
+  std::string Replacement =
+      ("std::numeric_limits<" +
----------------
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();
```


================
Comment at: clang-tidy/readability/NumericalCostantsToMaxIntCheck.cpp:107
+      << FixItHint::CreateReplacement(
+             SourceRange(Lit->getBeginLoc().getLocWithOffset(-1),
+                         Lit->getEndLoc()),
----------------
I think its better to create the source-range before to deal with exceptional cases (invalid ranges, macros)


================
Comment at: docs/clang-tidy/checks/list.rst:12
    abseil-redundant-strcat-calls
-   abseil-string-find-startswith
    abseil-str-cat-append
----------------
spurious change, does it still happen on master? i thought this was ordered at some point already


================
Comment at: docs/clang-tidy/checks/list.rst:219
    portability-simd-intrinsics
+   readability-NumericalCostantsToMaxInt
    readability-avoid-const-params-in-decls
----------------
this does not follow the naming convention (hyphens and not CamelCase)


================
Comment at: docs/clang-tidy/checks/readability-numerical-costants-to-max-int.rst:4
+readability-numerical-costants-to-max-int
+==================================
+
----------------
please make `===` full length.


================
Comment at: docs/clang-tidy/checks/readability-numerical-costants-to-max-int.rst:10
+
+If necessary it adds the library 'limits'.
+
----------------
This line is slightly inaccurate. The library is the standard library, `s/library/header-file`


================
Comment at: docs/clang-tidy/checks/readability-numerical-costants-to-max-int.rst:37
+  
\ No newline at end of file

----------------
Please remove the empty lines at the end


================
Comment at: test/clang-tidy/readability-numerical-costants-to-max-int.cpp:9
+
+unsigned const long Uval2 = -1;
+// CHECK-MESSAGES: :[[@LINE-1]]:30: warning: use 'std::numeric_limits<unsigned long>::max()' instead of '-1' for unsigned constant int [readability-numerical-costants-to-max-int]
----------------
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.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52892