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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

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.



================
Comment at: clang-tidy/readability/NumericalCostantsToMaxIntCheck.cpp:22
+namespace {
+class NumericalConstCheckPPCallbacks : public PPCallbacks {
+public:
----------------
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


================
Comment at: clang-tidy/readability/NumericalCostantsToMaxIntCheck.cpp:90
+  const auto *Lit = Result.Nodes.getNodeAs<IntegerLiteral>("Lit");
+  if (Decl == nullptr || Lit == nullptr)
+    return;
----------------
Is failure here expected? The matcher should match both nodes simultanously, if so please use an `assert` instead to detect unexpected failures


================
Comment at: docs/clang-tidy/checks/readability-numerical-costants-to-max-int.rst:16
+
+unsigned const long int x = -1;
+
----------------
please indent that code, otherwise its is rendered incorrect


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52892