[PATCH] D10892: warning when inner condition is identical

[Kim Gräsman]

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