[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