[PATCH] [clang-tidy] Assert related checkers

Alexander Kornienko alexfh at google.com
Wed Feb 4 11:35:23 PST 2015


Nice checks! Could you run it over LLVM/Clang and see whether they finds something?

Also, a couple of comments below.


================
Comment at: clang-tidy/misc/AssertSideEffectCheck.cpp:21
@@ +20,3 @@
+
+using namespace clang;
+using namespace ast_matchers;
----------------
This one is not needed, as the rest of the file is inside `namespace clang {}`. Let's just leave `using namespace clang::ast_matchers;` here.

================
Comment at: clang-tidy/misc/AssertSideEffectCheck.cpp:33
@@ +32,3 @@
+    UnaryOperator::Opcode OC = Op->getOpcode();
+    RetVal = OC == UO_PostInc || OC == UO_PostDec || OC == UO_PreInc ||
+             OC == UO_PreDec;
----------------
AFAIU, RetVal can never be changed below. So why not just return here? Same is valid for other cases.

================
Comment at: clang-tidy/misc/AssertSideEffectCheck.cpp:91
@@ +90,3 @@
+  Finder->addMatcher(
+      stmt(anyOf(conditionalOperator(hasCondition(
+                     hasDescendant(expr(hasSideEffect(CheckFunctionCalls))))),
----------------
Does it work if you pull out the common part:
  auto ConditionWithSideEffect = 
    hasCondition(hasDescendant(expr(hasSideEffect(CheckFunctionCalls)))));
  Finder->addMatcher(
        stmt(anyOf(conditionalOperator(ConditionWithSideEffect), ifStmt(ConditionWithSideEffect))));

?

================
Comment at: clang-tidy/misc/AssertSideEffectCheck.cpp:109
@@ +108,3 @@
+
+  // Check if this macro is an assert
+  if (std::find(AssertList.begin(), AssertList.end(), MacroName) ==
----------------
nit: Please finish statements with a period It makes comments look nicer

================
Comment at: test/clang-tidy/misc-assert-side-effect.cpp:11
@@ +10,3 @@
+#define assert(x)                                                              \
+  if (!(x))                                                                    \
+  abort()
----------------
Can you add a conditional operator-based implementation for testing?

http://reviews.llvm.org/D7375

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/






More information about the cfe-commits mailing list