[PATCH] [clang-tidy] Assert related checkers

[Alexander Kornienko]

Thanks for addressing the comments. A few comments more below and now I managed to review the second check as well.


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

2. I'd prefer to put the definition inside the condition:

  if (const auto *OpCallExpr = llvm::dyn_cast<CXXOperatorCallExpr>(E)) {
    ...


================
Comment at: clang-tidy/misc/AssertSideEffectCheck.cpp:49
@@ +48,3 @@
+             OpKind == OO_MinusEqual || OpKind == OO_StarEqual ||
+             OpKind == OO_SlashEqual || OpKind == OO_SlashEqual ||
+             OpKind == OO_PipeEqual || OpKind == OO_CaretEqual ||
----------------
Two comparisons with OO_SlashEqual.

================
Comment at: clang-tidy/misc/AssertSideEffectCheck.cpp:56
@@ +55,3 @@
+
+    bool RetVal = CheckFunctionCalls;
+    if (const auto *FuncDecl = CExpr->getDirectCallee())
----------------
Maybe s/RetVal/Result/? It's the same length, but it's a word.

================
Comment at: clang-tidy/misc/AssertSideEffectCheck.cpp:58
@@ +57,3 @@
+    if (const auto *FuncDecl = CExpr->getDirectCallee())
+      if (const auto *MethDecl = llvm::dyn_cast<CXXMethodDecl>(FuncDecl))
+        RetVal &= !MethDecl->isConst();
----------------
`MethodDecl` is just two letters more, but reads better.

================
Comment at: clang-tidy/misc/AssertSideEffectCheck.cpp:63
@@ +62,3 @@
+
+  return llvm::isa<CXXNewExpr>(E) || llvm::isa<CXXDeleteExpr>(E) ||
+         llvm::isa<CXXThrowExpr>(E);
----------------
Here and elsewhere: I think, dyn_cast, isa and StringRef are imported in either clang or global namespace. Please check if the code compiles without explicit qualifiers.

================
Comment at: clang-tidy/misc/AssertSideEffectCheck.cpp:74
@@ +73,3 @@
+    : ClangTidyCheck(Name, Context),
+      CheckFunctionCalls(Options.get("CheckFunctionCalls", false)),
+      RawAssertList(Options.get("AssertList", "assert")) {
----------------
Why is this option needed? Why "false" is the right default value?

================
Comment at: clang-tidy/misc/AssertSideEffectCheck.cpp:75
@@ +74,3 @@
+      CheckFunctionCalls(Options.get("CheckFunctionCalls", false)),
+      RawAssertList(Options.get("AssertList", "assert")) {
+  llvm::StringRef SR = RawAssertList;
----------------
The fact that it's a list can be conveyed by just using a plural, e.g. AssertMacros.

================
Comment at: clang-tidy/misc/AssertSideEffectCheck.cpp:77
@@ +76,3 @@
+  llvm::StringRef SR = RawAssertList;
+  SR.split(AssertList, " ", -1, false);
+}
----------------
It seems that comma-separated lists are more common.

================
Comment at: clang-tidy/misc/StaticAssertCheck.cpp:70
@@ +69,3 @@
+  llvm::StringRef MacroName = Lexer::getImmediateMacroName(
+      AssertExpansionLoc, ASTCtx->getSourceManager(), Opts);
+
----------------
s/ASTCtx->getSourceManager()/SM/

================
Comment at: clang-tidy/misc/StaticAssertCheck.cpp:83
@@ +82,3 @@
+
+  FixItHint NameHint, AssertExprRootHint, SLitHint, ArgHint;
+  if (AssertLoc.isValid() && !AssertLoc.isMacroID()) {
----------------
I'd replace these with a `SmallVector<FixItHint, 4>`.

================
Comment at: clang-tidy/misc/StaticAssertCheck.cpp:90
@@ +89,3 @@
+    if (AssertExprRoot) {
+      AssertExprRootHint = FixItHint::CreateRemoval(
+          SourceRange(AssertExprRoot->getOperatorLoc()));
----------------
I think, in case there's a message, the fixes should be made as follows:
  * insert a comma right after the end of the LHS (matters if there is a line break or a comment after the LHS)
  * remove the "&&" and any whitespace after it
  * don't touch the message at all.

In case there is no message originally, just insert a ", \"\"" right before the closing brace.

WDYT?

================
Comment at: clang-tidy/misc/StaticAssertCheck.cpp:107
@@ +106,3 @@
+                Buffer->getBufferStart(), BufferPos, Buffer->getBufferEnd());
+    Lexer.SetCommentRetentionState(false);
+
----------------
Why do you care about retention of comments? Does it matter?

================
Comment at: clang-tidy/misc/StaticAssertCheck.cpp:109
@@ +108,3 @@
+
+    while (!Lexer.LexFromRawLexer(Token) && Token.isNot(tok::l_paren))
+      ;
----------------
Do you expect to meet anything between "assert" and "("? Maybe just use  Lexer::getLocForEndOfToken and start the search of parentheses from after the "assert" token?

================
Comment at: clang-tidy/misc/StaticAssertCheck.cpp:113
@@ +112,3 @@
+    unsigned int ParenCount = 1;
+    while (ParenCount && !Lexer.LexFromRawLexer(Token)) {
+      if (Token.is(tok::l_paren))
----------------
Please pull the search of matching parentheses in a function. Probably, together with the Lexer, Buffer and Token variables.

================
Comment at: clang-tidy/misc/StaticAssertCheck.cpp:116
@@ +115,3 @@
+        ++ParenCount;
+      if (Token.is(tok::r_paren))
+        --ParenCount;
----------------
nit: I'd use `else if` to emphasize that these cases are mutually exclusive.

================
Comment at: clang-tidy/misc/StaticAssertCheck.h:1
@@ +1,2 @@
+//===--- StaticAssertCheck.h - clang-tidy -----------------------*- C++ -*-===//
+//
----------------
It's fine for now, but generally I'd prefer if you sent one patch per check. It would be easier to review and the review of each patch wouldn't block the other one.

Thanks!

http://reviews.llvm.org/D7375

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