NoQ created this revision.
NoQ added reviewers: alexfh, gribozavr2, aaron.ballman, xazax.hun, vsavchenko.
Herald added subscribers: martong, mgehre, rnkovacs.
NoQ requested review of this revision.
Herald added a project: clang-tools-extra.
If the loop condition is a value of an instance variable, a property value, or
a message result value, it's a good indication that the loop is not infinite
and we have a really hard time proving the opposite so suppress the warning.
Most of the newly added tests were already passing before the patch; the only
actually new test is `testArrayCountProperty()`; it was sufficient to either
add the `ObjCMessageExpr` case or the `ObjCPropertyRefExpr` case to suppress
it. I added all three though, and added a bunch of other tests, because the
reason they were passing previously was relatively bad and I want to make sure
things keep working even when these other reasons are eliminated.
For example, all tests with `...WithConstant` in their name were passing
because the checker was unable to identify the condition variable. I guess this
heuristic eliminates some intentionally infinite loops and guarantees that the
warning message always makes sense but I can't say that I fully understand it.
Other tests were passing because the checker notices a non-integral-type
variable (`arr`) in the condition. This restriction too can easily lead to
false negatives so I can't really get behind making it permanent.
The only reason it wasn't noticing `arr` in the `testArrayCountProperty()` test
was that `OpaqueValueExpr` was hiding it from the checker due to how its
`children()` are empty (don't include its `getSourceExpr()`!). Namely, the AST
generated for the property access in this test is as follows:
PseudoObjectExpr 0x7fb33d8b10c0 <col:10, col:14> 'NSUInteger':'unsigned long'
|-ObjCPropertyRefExpr 0x7fb33d8b1060 <col:10, col:14> '<pseudo-object type>'
lvalue objcproperty Kind=PropertyRef Property="count" Messaging=Getter
| `-OpaqueValueExpr 0x7fb33d8b1048 <col:10> 'NSArray *'
| `-ImplicitCastExpr 0x7fb33d8b0fe0 <col:10> 'NSArray *' <LValueToRValue>
| `-DeclRefExpr 0x7fb33d8b0fc0 <col:10> 'NSArray *' lvalue Var
0x7fb33d8b0e08 'arr' 'NSArray *'
|-OpaqueValueExpr 0x7fb33d8b1048 <col:10> 'NSArray *'
| `-ImplicitCastExpr 0x7fb33d8b0fe0 <col:10> 'NSArray *' <LValueToRValue>
| `-DeclRefExpr 0x7fb33d8b0fc0 <col:10> 'NSArray *' lvalue Var
0x7fb33d8b0e08 'arr' 'NSArray *'
`-ObjCMessageExpr 0x7fb33d8b1090 <col:14> 'NSUInteger':'unsigned long'
selector=count
`-OpaqueValueExpr 0x7fb33d8b1048 <col:10> 'NSArray *'
`-ImplicitCastExpr 0x7fb33d8b0fe0 <col:10> 'NSArray *' <LValueToRValue>
`-DeclRefExpr 0x7fb33d8b0fc0 <col:10> 'NSArray *' lvalue Var
0x7fb33d8b0e08 'arr' 'NSArray *'
You can see that all three `DeclRefExpr`s are shadowed by their respective
`OpaqueValueExpr`s. Both `ObjCPropertyRefExpr` and `ObjCMessageExpr` are the
give-aways, as well as the entire parent `PseudoObjectExpr`. Hence the patch.
Another solution I could have implemented would have been to actively traverse
`OpaqueValueExpr->getSourceExpr()` to make sure that the suppression keeps
working. I wasn't comfortable implementing it because I don't fully understand
`OpaqueValueExpr` (which isn't ObjC-specific, it shows up in normal C++ as well
IIRC) so I was worried about unexpected consequences.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D102294
Files:
clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp
clang-tools-extra/test/clang-tidy/checkers/bugprone-infinite-loop.mm
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-infinite-loop.mm
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-infinite-loop.mm
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-infinite-loop.mm
@@ -1,16 +1,28 @@
// RUN: %check_clang_tidy %s bugprone-infinite-loop %t -- -- -fblocks
+// RUN: %check_clang_tidy %s bugprone-infinite-loop %t -- -- -fblocks -fobjc-arc
+
+typedef __typeof(sizeof(int)) NSUInteger;
+
+@interface NSArray
++(instancetype)alloc;
+-(instancetype)init;
+@property(readonly) NSUInteger count;
+-(void)addObject: (id)anObject;
+@end
@interface I
-(void) instanceMethod;
+(void) classMethod;
+(int &) hiddenReferenceTo: (int &)x;
++(instancetype)alloc;
+-(instancetype)init;
@end
void plainCFunction() {
int i = 0;
int j = 0;
while (i < 10) {
- // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (i) are updated in the loop body [bugprone-infinite-loop]
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its con
j++;
}
}
@@ -47,3 +59,81 @@
return x;
}
@end
+
+void testArrayCount() {
+ NSArray *arr = [[NSArray alloc] init];
+ NSUInteger max_count = 10;
+ while ([arr count] < max_count) {
+ // No warning. Array count is updated on every iteration.
+ [arr addObject: [[I alloc] init]];
+ }
+}
+
+void testArrayCountWithConstant() {
+ NSArray *arr = [[NSArray alloc] init];
+ while ([arr count] < 10) {
+ // No warning. Array count is updated on every iteration.
+ [arr addObject: [[I alloc] init]];
+ }
+}
+
+void testArrayCountProperty() {
+ NSArray *arr = [[NSArray alloc] init];
+ NSUInteger max_count = 10;
+ while (arr.count < max_count) {
+ // No warning. Array count is updated on every iteration.
+ [arr addObject: [[I alloc] init]];
+ }
+}
+
+void testArrayCountPropertyWithConstant() {
+ NSArray *arr = [[NSArray alloc] init];
+ while (arr.count < 10) {
+ // No warning. Array count is updated on every iteration.
+ [arr addObject: [[I alloc] init]];
+ }
+}
+
+@interface MyArray {
+ @public NSUInteger _count;
+}
++(instancetype)alloc;
+-(instancetype)init;
+-(void)addObject: (id)anObject;
+
+-(void)populate;
+@end
+
+@implementation MyArray
+-(void)populate {
+ NSUInteger max_count = 10;
+ while (_count < max_count) {
+ // No warning. Array count is updated on every iteration.
+ [self addObject: [[I alloc] init]];
+ }
+}
+
+-(void)populateWithConstant {
+ while (_count < 10) {
+ // No warning. Array count is updated on every iteration.
+ [self addObject: [[I alloc] init]];
+ }
+}
+@end
+
+void testArrayCountIvar() {
+ MyArray *arr = [[MyArray alloc] init];
+ NSUInteger max_count = 10;
+ while (arr->_count < max_count) {
+ // No warning. Array count is updated on every iteration.
+ [arr addObject: [[I alloc] init]];
+ }
+}
+
+void testArrayCountIvarWithConstant() {
+ MyArray *arr = [[MyArray alloc] init];
+ while (arr->_count < 10) {
+ // No warning. Array count is updated on every iteration.
+ [arr addObject: [[I alloc] init]];
+ }
+}
Index: clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp
@@ -61,7 +61,9 @@
isChanged(LoopStmt, Var, Context);
// FIXME: Track references.
}
- } else if (isa<MemberExpr>(Cond) || isa<CallExpr>(Cond)) {
+ } else if (isa<MemberExpr>(Cond) || isa<CallExpr>(Cond) ||
+ isa<ObjCIvarRefExpr>(Cond) || isa<ObjCPropertyRefExpr>(Cond) ||
+ isa<ObjCMessageExpr>(Cond)) {
// FIXME: Handle MemberExpr.
return true;
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
