Extend IdenticalExprChecker to find more potential problems

[Jordan Rose]

Hi, Daniel. Can you split these two checks up into separate patches? I know they're pretty similar and have the same basic idea, but we like to have commits be limited to one new feature at a time. (Combining bitwise and logical operators into one is fine, though.) Sorry it's a bit more work for you to tease them apart.

Some comments:

+static bool isIdenticalStmt(const ASTContext &Ctx, const Stmt *Expr1,
+                            const Stmt *Expr2, bool IgnoreSideEffects = false);

I think it's probably worth changing the argument names too (to not say "Expr").


+  void checkComparissionOp(const BinaryOperator *B);

Typo: "Comparison"


+                     "Usage of identical expressions",

I would just say "Use"


+  // Split operators as long as we still have operators to split on. We will
+  // get called for every binary operator in an expression so there is no need
+  // to check every one against each other here, just the right most one with
+  // the others.

Clever! Too bad it comes out to an N^2 algorithm, but N is likely pretty small most of the time, and the nature of isIdenticalStmt


+  while (true) {
+     const BinaryOperator *B2 = dyn_cast<BinaryOperator>(LHS);
+     if (!B2)
+       break;

You can combine these into a single line:

while (const BinaryOperator *B2 = dyn_cast<BinaryOperator>(LHS))


+  if (Stmt1->getStmtClass() == Stmt::CompoundStmtClass) {
+    const CompoundStmt *CompStmt = cast<CompoundStmt>(Stmt1);

This is written:

if (const CompoundStmt *CompStmt = dyn_cast<CompoundStmt>(Stmt1))

(Also, good job handling this case!)


+                         "Identical expressions regardless of boolean value",

I'm not sure about this one. For one thing, the if-branch and else-branch aren't usually just expressions. How about just "Identical branches"? (Or you/we can continue hunting for a better alternative.)


Some missing test cases:

Should not warn: x == 4 && y == 5 || x == 4 && y == 6

Should warn: x ? "abc" : "abc"

Test cases with while, do, for, if, and return within the branches of an if. If you add the code, we need to test them! We could also not worry about this for now and just say the checker isn't powerful enough to handle them.

Why would that be useful? Because I'm a bit concerned about performance for the if-branch-check. Can you try running this over a large-ish project and let me know the before and after times of analyzing with this checker on? If it's a significant difference we should put the if-check under a separate checker name.

Thanks for working on this!
Jordan


On Jan 29, 2014, at 23:30 , Daniel Fahlgren <daniel at fahlgren.se> wrote:

> Hi,
> 
> This patch extends the identical expression checker to find more places
> where you have code duplication, or copy and paste errors. It will now
> find if you check the same thing multiple times in a logical expression.
> E.g.
> 
>  if (a == 1 || a == 1)
> 
> It will also detect if you have used the same symbol multiple times when
> using bitwise operators. E.g.
> 
>  int b = a | a;
> 
> The last addition is that it checks if both branches in an if-statement
> are identical. E.g.
> 
>  if (...)
>    a = 1;
>  else
>    a = 1;
> 
> 
> Since this is my first patch I'm sure I've forgotten something. Feedback
> is welcome.
> 
> 
> For revision 200460 of the llvm tree it actually gives the following
> warnings:
> 
> llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:428:54: warning:
> identical expressions on both sides of bitwise operator
>    result |= (icmp_eq ? (FoldMskICmp_Mask_AllZeroes |
>                          ~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
> llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:432:57: warning:
> identical expressions on both sides of bitwise operator
>                       : (FoldMskICmp_Mask_NotAllZeroes |
>                          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
> 
> llvm/lib/IR/ConstantFold.cpp:936:12: warning: true and false branches
> are identical
>      else if (isa<UndefValue>(C1)) 
>           ^
> 
> llvm/tools/clang/lib/Basic/Targets.cpp:981:22: warning: identical
> expressions on both sides of bitwise operator
>                     | ArchDefinePwr6 | ArchDefinePpcgr |
> ArchDefinePpcsq)
>                     ^ ~~~~~~~~~~~~~~
> llvm/tools/clang/lib/Basic/Targets.cpp:995:24: warning: identical
> expressions on both sides of bitwise operator
>                       | ArchDefinePwr6 | ArchDefinePpcgr |
> ArchDefinePpcsq)
>                       ^ ~~~~~~~~~~~~~~
> 
> 
> Best regards,
> Daniel Fahlgren
-------------- next part --------------
A non-text attachment was scrubbed...
Name: identical.patch
Type: text/x-patch
Size: 20445 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140130/d2bb2212/attachment.bin>
-------------- next part --------------
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits