[PATCH] D95519: [clang-tidy] bugprone-assert-side-effect: Warn on NSAssert by default.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added reviewers: alexfh, gribozavr2, vsavchenko.
NoQ added a project: clang-tools-extra.
Herald added subscribers: martong, Charusso, xazax.hun.
NoQ requested review of this revision.

This is basically standard Objective-C assert. While we could always pass it as an option, i believe it makes perfect sense to warn on it by default.

The same applies to `NSCAssert` which drops additional support for displaying Objective-C method information so it acts exactly like C `assert`.

I tested it on live code with actual Foundation headers and it seems to work correctly out of the box.

I'm also interested in making an even more agressive step in this direction and adding an option to accept any macro that ends with "...assert" suffix, case-insensitively. This would probably go under an off-by-default flag. Would this be acceptable?


https://reviews.llvm.org/D95519

Files:
  clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone-assert-side-effect.m


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-assert-side-effect.m
===================================================================
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-assert-side-effect.m
@@ -0,0 +1,53 @@
+// RUN: %check_clang_tidy %s bugprone-assert-side-effect %t
+
+int abort();
+
+ at interface NSObject
+ at end
+
+ at interface NSString
+ at end
+
+ at interface NSAssertionHandler
++ (NSAssertionHandler *)currentHandler;
+- handleFailureInMethod:(SEL)cmd object:(NSObject *)obj desc:(NSString *)desc;
+- handleFailureInFunction:(NSString *)desc;
+ at end
+
+#ifndef NDEBUG
+#define NSAssert(condition, description, ...)                                    \
+  do {                                                                           \
+    if (__builtin_expect(!(condition), 0)) {                                     \
+      [[NSAssertionHandler currentHandler] handleFailureInMethod:_cmd            \
+                                                          object:self            \
+                                                            desc:(description)]; \
+    }                                                                            \
+  } while (0);
+#define NSCAssert(condition, description, ...)                                     \
+  do {                                                                             \
+    if (__builtin_expect(!(condition), 0)) {                                       \
+      [[NSAssertionHandler currentHandler] handleFailureInFunction:(description)]; \
+    }                                                                              \
+  } while (0);
+#else
+#define NSAssert(condition, description, ...) do {} while (0)
+#define NSCAssert(condition, description, ...) do {} while (0)
+#endif
+
+ at interface I : NSObject
+- (void)foo;
+ at end
+
+ at implementation I
+- (void)foo {
+  int x = 0;
+  NSAssert((++x) == 1, @"Ugh.");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: side effect in NSAssert() condition discarded in release builds [bugprone-assert-side-effect]
+}
+ at end
+
+void foo() {
+  int x = 0;
+  NSCAssert((++x) == 1, @"Ugh.");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: side effect in NSCAssert() condition discarded in release builds [bugprone-assert-side-effect]
+}
Index: clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.cpp
@@ -72,7 +72,8 @@
                                              ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       CheckFunctionCalls(Options.get("CheckFunctionCalls", false)),
-      RawAssertList(Options.get("AssertMacros", "assert")) {
+      RawAssertList(Options.get("AssertMacros",
+                                "assert,NSAssert,NSCAssert")) {
   StringRef(RawAssertList).split(AssertMacros, ",", -1, false);
 }
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D95519.319539.patch
Type: text/x-patch
Size: 3016 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20210127/7ff33730/attachment-0001.bin>