ztamas marked 16 inline comments as done.
ztamas added inline comments.
================
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:20
+
+static const char LoopName[] = "forLoopName";
+static const char loopVarName[] = "loopVar";
----------------
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.
================
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]
----------------
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?
================
Comment at: test/clang-tidy/bugprone-too-small-loop-variable.cpp:116
+
+// TODO: handle those cases when the loop condition contains a floating point
expression
+void voidDoubleForCond() {
----------------
whisperity wrote:
> (Misc: You might want to check out D52730 for floats later down the line.)
In the meantime, I thought this float comparison is not a real use case. It
just came in my mind while I was trying to find out test cases. So I just
removed it. The referenced conversion check might catch it anyway.
================
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
----------------
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.
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
