[PATCH] [clang-tidy] Assert related checkers

[Alexander Kornienko]

Sorry for the long delay. This looks better, but still a few comments.


================
Comment at: clang-tidy/misc/AssertSideEffectCheck.cpp:44
@@ +43,3 @@
+  if (const auto *CExpr = dyn_cast<CallExpr>(E)) {
+    if (const auto *OpCallExpr = dyn_cast<CXXOperatorCallExpr>(E)) {
+      OverloadedOperatorKind OpKind = OpCallExpr->getOperator();
----------------
So why do you need to place this `if` inside the `if (.. = dyn_cast<CallExpr>(E))`?

================
Comment at: clang-tidy/misc/AssertSideEffectCheck.h:24
@@ +23,3 @@
+/// The condition of \c assert() is evaluated only in debug builds so a
+/// condition
+/// with side effect can cause different behaviour in debug / relesase builds.
----------------
nit: The comment is now mis-formatted.

================
Comment at: clang-tidy/misc/AssertSideEffectCheck.h:28
@@ +27,3 @@
+/// There are two options:
+/// - AssertMacros: A string containing the names of the assert macros the user
+///   wants to be checked.
----------------
How about making the comment a bit less wordy:
    - AssertMacros: A comma-separated list of the names of assert macros to be checked.
?

================
Comment at: clang-tidy/misc/AssertSideEffectCheck.h:32
@@ +31,3 @@
+///   like whitespace, semicolon.
+/// - CheckFunctionCalls: Whether to check function calls. Disabled by default
+///   because it can increase the number of false positive warnings by reporting
----------------
This doesn't explain much. "Checking function calls" is not what this option controls. AFAIU, it makes the check treat non-const member calls as though they produce side effects.

I also don't understand what do you mean by "reporting free functions". Maybe add a motivating example?

================
Comment at: clang-tidy/misc/StaticAssertCheck.cpp:83
@@ +82,3 @@
+
+  SmallVector<FixItHint, 4> FixItHints(4);
+  SourceLocation LastParenLoc;
----------------
Creating an empty vector and then using `push_back()` to add whatever fixes you need looks like a more elegant solution.

================
Comment at: clang-tidy/misc/StaticAssertCheck.cpp:103
@@ +102,3 @@
+  diag(AssertLoc, "found assert() that could be replaced by static_assert()")
+      << FixItHints[0] << FixItHints[1] << FixItHints[2] << FixItHints[3];
+}
----------------
Starting from r230495, you can insert FixItHints directly.

================
Comment at: clang-tidy/misc/StaticAssertCheck.cpp:107
@@ +106,3 @@
+const SourceLocation
+StaticAssertCheck::getLastParenLoc(ASTContext *const ASTCtx,
+                                   const SourceLocation AssertLoc) {
----------------
As I probably mentioned before, top-level constness doesn't make sense in function parameter types. It is just an implementation detail that is not interesting to the API users, but it makes the interface less readable.

================
Comment at: test/clang-tidy/misc-assert-side-effect.cpp:1
@@ +1,2 @@
+
+// RUN: $(dirname %s)/check_clang_tidy.sh %s misc-assert-side-effect %t -config="{CheckOptions: [{key: misc-assert-side-effect.CheckFunctionCalls, value: 1}, {key: misc-assert-side-effect.AssertMacros, value: 'assert,my_assert'}]}" --
----------------
Please remove the empty line.

http://reviews.llvm.org/D7375

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