Cppcheck reports "Logical disjunction always evaluates to true" in sdext module

classic Classic list List threaded Threaded
2 messages Options
julien2412 julien2412
Reply | Threaded
Open this post in threaded view
|

Cppcheck reports "Logical disjunction always evaluates to true" in sdext module

Hello,

Cppcheck reports this in sdext module :
[source/minimizer/optimizerdialog.cxx:276]: (warning) Logical disjunction always evaluates to true: nNewStep >= 0 || nNewStep <= 4
Here is the line :
if ( ( nNewStep != mnCurrentStep ) && ( ( nNewStep <= MAX_STEP ) || ( nNewStep >= 0 ) ) )

Shouldn't it be :
if ( ( nNewStep != mnCurrentStep ) && ( nNewStep <= MAX_STEP ) && ( nNewStep >= 0 ) )
?
Moreover, if I'm right, should we additionnally throw an exception (which one ?) if "nNewStep" is not between 0 and MAX_STEP

If ok for just the condition change, I can commit and push on master (what about 3.5 ?)
About the exception, I don't know what to put.

Julien.
Petr Mladek Petr Mladek
Reply | Threaded
Open this post in threaded view
|

Re: [PUSHED:master,3-5] Cppcheck reports "Logical disjunction always evaluates to true" in sdext module

julien2412 píše v Ne 18. 03. 2012 v 10:24 -0700:

> Hello,
>
> Cppcheck reports this in sdext module :
> [source/minimizer/optimizerdialog.cxx:276]: (warning) Logical disjunction
> always evaluates to true: nNewStep >= 0 || nNewStep <= 4
> Here is the line :
> if ( ( nNewStep != mnCurrentStep ) && ( ( nNewStep <= MAX_STEP ) || (
> nNewStep >= 0 ) ) )
>
> Shouldn't it be :
> if ( ( nNewStep != mnCurrentStep ) && ( nNewStep <= MAX_STEP ) && ( nNewStep
> >= 0 ) )

Yup, it makes perfect sense. I have pushed the change into both master
and 3-5 branch, see
http://cgit.freedesktop.org/libreoffice/core/commit/?id=9481eb32e76568fc675982479cc07c57c6a7cb39
http://cgit.freedesktop.org/libreoffice/core/commit/?h=libreoffice-3-5&id=8eb5b36284c3a6c6535508a3f0651be1b5ada2dc


> Moreover, if I'm right, should we additionnally throw an exception (which
> one ?) if "nNewStep" is not between 0 and MAX_STEP

I would leave it as is. The affected method is
OptimizerDialog::SwitchPage. IMHO, it is better to do not switch pages
instead of canceling the whole operation with an useless exception.


Best Regards,
Petr

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice