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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
