NIRALUser
IntensityMotionCheckPanel: Most likely incorrect test in ResultUpdate() function
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
Most likely incorrect test:
if(
(
(
(!qcResult.Get_result() & InterlaceWiseCheckBit) == InterlaceWiseCheckBit
)
||
(
!(
this->GetProtocol().GetSliceCheckProtocol().bQuitOnCheckFailure ||
this->GetProtocol().GetInterlaceCheckProtocol().bQuitOnCheckFailure
)
)
)
&& this->GetProtocol().GetGradientCheckProtocol().bCheck
)
vs
similar but most likely correct test
if(
(
(
!(
(qcResult.Get_result() & InterlaceWiseCheckBit) == InterlaceWiseCheckBit
)
)
||
(!
(
this->GetProtocol().GetSliceCheckProtocol().bQuitOnCheckFailure ||
this->GetProtocol().GetInterlaceCheckProtocol().bQuitOnCheckFailure
)
)
)
)
Then, based on my previous comment I think the correct test should be:
if(
(
!( // <------- Logical not moved outside
(qcResult.Get_result() & InterlaceWiseCheckBit) == InterlaceWiseCheckBit
)
||
(
!(
this->GetProtocol().GetSliceCheckProtocol().bQuitOnCheckFailure ||
this->GetProtocol().GetInterlaceCheckProtocol().bQuitOnCheckFailure
)
)
)
&& this->GetProtocol().GetGradientCheckProtocol().bCheck
)
Which in turn can be rewritten as this more readable version:
bool doInterlaceWiseCheck = (qcResult.Get_result() & InterlaceWiseCheckBit) == InterlaceWiseCheckBit;
bool doQuitOnCheckFailure =
this->GetProtocol().GetSliceCheckProtocol().bQuitOnCheckFailure ||
this->GetProtocol().GetInterlaceCheckProtocol().bQuitOnCheckFailure;
if(
( !doInterlaceWiseCheck || !doQuitOnCheckFailure )
&& this->GetProtocol().GetGradientCheckProtocol().bCheck
)
{
// ...
}
Thanks JC, I think you are correct. DTIPrep has (in parts) some really ugly code (2 postdocs who weren't the best coders worked on it), sorry!
Hi @styner ,
No need to be sorry. It is nature of the work.
Improving our processes, providing education and resources to increase quality of our softwares, and also including software maintenance in our grants and funding requests are all helpful actions :smile:
The code being open-source with a permissive license, and available on GitHub with the option of doing pull requests is one of the many steps in the right direction.
I think some of the take home messages would be:
- to care for build warnings and try to either address them or explicitly disable them. They often reveals useful information.
- add tests
Thanks again for reviewing and integrating these changes. Jc