SV: Extend IdenticalExprChecker to find more potential problems

[Daniel Marjamäki]

Hello!

I'd recommend that we skip duplicate branches.

However I have not seen any such code:

> ....
> } else if (bar()) {
>         /* no action needed */
> } else {
>         /* FIXME: Implement this. */
> } 

so if you want to warn here, then I can't point out any noise.

Best regards,
Daniel Marjamäki

..................................................................................................................
Daniel Marjamäki Senior Engineer
Evidente ES East AB  Warfvinges väg 34  SE-112 51 Stockholm  Sweden

Mobile:                 +46 (0)709 12 42 62
E-mail:                 Daniel.Marjamaki at evidente.se

www.evidente.se

________________________________________
Från: Daniel Fahlgren [daniel at fahlgren.se]
Skickat: den 11 februari 2014 16:30
Till: Daniel Marjamäki
Cc: Jordan Rose; cfe-commits at cs.uiuc.edu
Ämne: Re: Extend IdenticalExprChecker to find more potential problems

Hi Daniel,

On Tue, 2014-02-11 at 07:56 +0000, Daniel Marjamäki wrote:
> Thank you Daniel for extending the IdenticalExprChecker. Imho these
> checks are very useful.
>
> About such code:
>
> if (foo()) {
>         doSomething();
> } else if (bar()) {
>         /* no action needed */
> } else {
>         /* FIXME: Implement this. */
> }
>
> You could warn here. However there will some noise.
>
> In my opinion it is not very rare to have identical blocks of code in
> if and else blocks by intention. My feeling is that for instance MISRA
> enforce some identical blocks because there must always be an empty
> else if the "else" logic should be to do nothing.

I'm fine with changing the behavior if people think there will be too
much noise. I have no strong opinion towards one way or another. As
Jordan pointed out, it is easy to have special handling for two empty
branches.

In the MISRA example, why is the true block also empty? Are you required
to "handle" the return value from a function but not actually handle it?
>
> Here is one real code example where there are identical blocks by
> intention. The TODO means that the code could be enhanced but it's
> fine for now.
> https://github.com/danmar/cppcheck/blob/6bfd4af5/lib/checkunusedvar.cpp#L528

That is harder to distinguish from a copy-paste error. I'm not sure how
to handle that, or if it is even possible.

> I have not tried your patch yet though. I have access to some misra
> compatible code I could test it on.

Best regards,
Daniel Fahlgren