JonasToth added inline comments.
================
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:20
+
+static const char LoopName[] = "forLoopName";
+static const char loopVarName[] = "loopVar";
----------------
ztamas wrote:
> JonasToth wrote:
> > Please move these variable in the matcher function and make them
> > `StringRef` instead of `const char[]`.
> These variables are used not only inside the matcher function but also in the
> check() function.
>
I didn't see that, but they should still be `StringRef` as type.
================
Comment at: test/clang-tidy/bugprone-too-small-loop-variable.cpp:6
+void voidBadForLoop() {
+ for (int i = 0; i < size(); ++i) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: loop variable has a narrower
type ('int') than the type ('long') of termination condition
[bugprone-too-small-loop-variable]
----------------
ztamas wrote:
> JonasToth wrote:
> > please add tests where the rhs is a literal.
> Do you mean tests like voidForLoopWithLiteralCond()?
> Is it worth to add more tests like that?
I didn't see it. In principle yes, but i would like to see a test with a bigger
number then iterateable (maybe there is a frontend warning for that?). If there
is no warning, this should definitely be implemented here (possible follow up)
or even become a frontend warning.
================
Comment at: test/clang-tidy/bugprone-too-small-loop-variable.cpp:152
+void voidForLoopWithEnumCond() {
+ for (short i = eSizeType::START; i < eSizeType::END; ++i) {
+ } // no warning
----------------
ztamas wrote:
> JonasToth wrote:
> > It is possible to specify the underlying type of an `enum`.
> > For the case `enum eSizeType2 : int;` the problem does occur as well. I
> > think especially this case is tricky to spot manually and should be handled
> > too. What do you think?
> Hmm, it can be handled I think. However, I'm not sure how often it is, that
> an enum has an item value bigger than 32767 (short) or 65535 (unsigned short)
> or another even bigger value.
> For now, I think it's better to just ignore these cases to avoid false
> positives and keep the first version of the check simple. The scope can be
> extended anytime later I think.
`enum` values can become very big for flag-enums
```
enum Foo {
value1 = 1 << 1;
// ...
value 24 = 1 << 24;
};
```
You should still add a test for a `enum` with specified underlying type to
ensure, there is no noise.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53974
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
