IntensityMotionCheckPanel: Most likely incorrect test in ResultUpdate() function #44
Comments
|
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
)
{
// ...
} |
…logic issue Based on the operator precedence [1], adding parenthesis around "!qcResult.Get_result()" would be the right things to do address the warning, that said this is incorrect since the value returned by "Get_result()" always evaluates to 1 or 0. [1] http://en.cppreference.com/w/c/language/operator_precedence See NIRALUser#44 This commit improves readability introducing intermediate variables and move the logical not operator outside the bitwise test. This commit fixes 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 ) || ^ ```
|
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 😄 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:
Thanks again for reviewing and integrating these changes. |
While looking into addressing the following warning:
associated with this code:
DTIPrep/src/IntensityMotionCheckPanel.cxx
Lines 3979 to 3983 in d764409
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 byGet_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:
DTIPrep/src/IntensityMotionCheckPanel.cxx
Lines 4245 to 4248 in d764409
The text was updated successfully, but these errors were encountered: