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/
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
