IntensityMotionCheckPanel: Most likely incorrect test in ResultUpdate() function

[jcfr]

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:

DTIPrep/src/IntensityMotionCheckPanel.cxx

Lines 3979 to 3983 in d764409

	if( ( ( (!qcResult.Get_result() & InterlaceWiseCheckBit) == InterlaceWiseCheckBit ) ||
	(!(this->GetProtocol().GetSliceCheckProtocol().bQuitOnCheckFailure ||
	this->GetProtocol().GetInterlaceCheckProtocol().bQuitOnCheckFailure) ) ) &&
	this->GetProtocol().GetGradientCheckProtocol().bCheck )
	{

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:

DTIPrep/src/IntensityMotionCheckPanel.cxx

Lines 4245 to 4248 in d764409

	if( ( (!( (qcResult.Get_result() & InterlaceWiseCheckBit) == InterlaceWiseCheckBit) ) ||
	(!(this->GetProtocol().GetSliceCheckProtocol().bQuitOnCheckFailure ||
	this->GetProtocol().GetInterlaceCheckProtocol().bQuitOnCheckFailure) ) ) )
	{

[jcfr]

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
            ) 

      ) 

   ) 
)

[jcfr]

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 


)

[jcfr]

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 
)
  {
  // ...
  }

[styner]

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!

[jcfr]

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:

• 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