Extend IdenticalExprChecker to find more potential problems

Daniel Fahlgren daniel at fahlgren.se
Tue Feb 11 07:30:57 PST 2014


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




More information about the cfe-commits mailing list