kimgr added a comment. Some style nits and a question around the purpose of `Sr`. It doesn't appear to do what I thought it would.
I'll leave further review in the hands of owners, I'm just passing through. ================ Comment at: lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp:119 @@ +118,3 @@ + const IfStmt *InnerIf = dyn_cast<IfStmt>(*CS->body_begin()); + if (InnerIf && isIdenticalStmt(AC->getASTContext(), I->getCond(), InnerIf->getCond(), false)) { + SourceRange Sr = I->getCond()->getSourceRange(); ---------------- Could you say /*IgnoreSideEffects=*/ false for the last param, so it's more obvious that conditions with side effects are not considered? ================ Comment at: lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp:120 @@ +119,3 @@ + if (InnerIf && isIdenticalStmt(AC->getASTContext(), I->getCond(), InnerIf->getCond(), false)) { + SourceRange Sr = I->getCond()->getSourceRange(); + PathDiagnosticLocation ELoc(InnerIf->getCond(), BR.getSourceManager(), AC); ---------------- Can you name this after its purpose? Something like `OriginalCondRange`...? ================ Comment at: lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp:125 @@ +124,3 @@ + "inner condition is identical to previous condition", + ELoc, Sr); + } ---------------- Is the Sr used for the note, and intended to point to the first if-statement? In your posted output the warning and the note both have the same location, and it would be neat if they pointed to the respective stmts. ================ Comment at: test/Analysis/identical-expressions.cpp:1544 @@ +1543,3 @@ + + // todo: should warn here. the warning is currently not written because there + // is code between the conditions. ---------------- s/todo/FIXME/ Use proper sentence casing in comments. s/written/emitted/ ? ================ Comment at: test/Analysis/identical-expressions.cpp:1554 @@ +1553,3 @@ +void test_nowarn_inner_if_1(int x) { + // dont warn when condition has side effects + if (x++ == 1) { ---------------- s/dont/Don't/ End sentence with a full stop. ================ Comment at: test/Analysis/identical-expressions.cpp:1560 @@ +1559,3 @@ + + // dont warn when x is changed before inner condition + if (x < 10) { ---------------- s/dont/Don't/ and end with a full stop. http://reviews.llvm.org/D10892 _______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits