DTIPrep icon indicating copy to clipboard operation
DTIPrep copied to clipboard

IntensityMotionCheckPanel: Most likely incorrect test in ResultUpdate() function

Open jcfr opened this issue 7 years ago • 5 comments

While looking into addressing the following warning:

tmp/DTIPrep/src/IntensityMotionCheckPanel.cxx: In member function ‘void IntensityMotionCheckPanel::ResultUpdate()’:
/tmp/DTIPrep/src/IntensityMotionCheckPanel.cxx:3978:33: warning: suggest parentheses around operand of ‘!’ or change ‘&’ to ‘&&’ or ‘!’ to ‘~’ [-Wparentheses]
   if( ( ( (!qcResult.Get_result()  & InterlaceWiseCheckBit) == InterlaceWiseCheckBit ) ||
                                 ^

associated with this code:

https://github.com/NIRALUser/DTIPrep/blob/d76440973fb9b2afd223597080dccdc8a1447775/src/IntensityMotionCheckPanel.cxx#L3979-L3983

I suspect something is wrong.

Indeed, based on the operator precedence [1], adding parenthesis around !qcResult.Get_result() is the right thing to do to address the warning, that said the check should be reviewed since the value returned by Get_result() always evaluates to 1 or 0.

[1] http://en.cppreference.com/w/c/language/operator_precedence

Looking at this test in an other part of the code hints on what should be the correct implementation:

https://github.com/NIRALUser/DTIPrep/blob/d76440973fb9b2afd223597080dccdc8a1447775/src/IntensityMotionCheckPanel.cxx#L4245-L4248

jcfr avatar Sep 01 '17 10:09 jcfr