Extend IdenticalExprChecker to find more potential problems

[Jordan Rose]

On Feb 2, 2014, at 23:16 , Daniel Fahlgren <daniel at fahlgren.se> wrote:

> Hi, Jordan.
> 
> On Thu, 2014-01-30 at 09:47 -0800, Jordan Rose wrote:
>> 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.
> 
> No problem. I have fixed your comments with the two attached patches.
> if_stmt adds support to test if both branches are identical and should
> be applied first. bin_op adds support to find identical expressions when
> using binary operators.
> 
>> +  // 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
> 
> Thanks. I thought about a better way to fix this when I implemented it,
> but couldn't find a good one. First, somehow, sorting the expressions
> and check only adjacent ones will probably be slower in practice due to
> the overhead of sorting.
> 
>> 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.
> 
> Test cases added.
> 
>> 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.
> 
> I ran some tests with php 5.5.8 and openssl 1.0.1f. The tests were done
> on my laptop with a ssd and 8GB of ram, so the numbers should be cpu
> bound. Between each run I completely removed the directory and
> reconfigured to make sure conditions were as similar as possible. The
> checker was modified t only check the branches of an if statement, not
> the binary operators.
> 
> Adding the check when running scan-build on php increased the time by
> 0.2%. On openssl it actually decreased by 0.15% instead. I think the
> background tasks of my otherwise idle desktop has more influence on the
> numbers than the test.
> 
> php 5.5.8 base run:
> 
> real  53m44.366s
> user  51m31.245s
> sys    1m11.288s
> 
> php 5.5.8 with checking if stmts:
> 
> real  53m51.311s
> user  51m35.985s
> sys    1m12.949s
> 
> openssl 1.0.1f base run:
> 
> real  11m40.819s
> user  10m39.124s
> sys    0m49.091s
> 
> openssl 1.0.1f with checking if stmts:
> 
> real  11m39.783s
> user  10m38.536s
> sys    0m48.879s
> 
> Best regards,
> Daniel Fahlgren

Thanks for running the timing checks. I agree that this seems pretty benign.

I thought of another controversial test case:

if (foo()) {
	doSomething();
} else if (bar()) {
	/* do nothing */
} else if (baz()) {
	/* do nothing */
} else {
	error();
}

It'd be pretty easy to just not check empty compound-statements. What do you think?

More test cases for the logical op checker:

x == 4 && y == 5 || x == 4
x == 4 || y == 5 && x == 4
x == 4 || x == 4 && y == 5

All of these would be fair to warn on, but we can probably get away with not for the first pass. If we already support one, though, should we add it as a regression test?

Finally, the typo "comparision" is still in the patch. :-)

Jordan

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140203/2c2625d5/attachment.html>