ITK/Release 4/Modularization/Code Reviews/Checklist

From KitwarePublic
Jump to navigationJump to search

This page defines the check list to be used by reviewers during the process of reviewing the files in ITKv4.

Major Topics

The reviewer will verify the file for compliance with the following major topics

  • Coding Style
  • Const correctness
  • Mini-Pipelines
  • Progress Reporting
  • Multi-Threading
  • Member variables holding to memory

The following sections specify the details to be evaluated with each one of these major topics.

Coding Style

Despite the fact that coding style is verified regularly with KStyle, there are still many details that escape that verification. The following list specifies what elements must be verified by the reviewer.

  • Filename must match class name
  • Files must contain a single class
  • Implementation of methods must be in the .txx or .cxx files, not the .h files
  • All files must have the Copyright Header at the top.
  • #define for class name in the .h and .txx files. __classname_h and __classname_txx
  • Doxygen documentation
  • Brief class doxygen description
  • namespace itk
  • Complete class doxygen description
  • Classes that use SmartPointers must have
    • Constructor/Destructor protected
    • Copy Constructor : private and not implemented
    • Operator= : private and not implemented
  • No acronyms in class name or method names
  • No unnecessary headers #included
  • Justify every public method
  • All member variables must be private
  • Use Set/Get Macros (macros should call Modified() to ensure pipeline data is updated)
  • If deriving from itkObject
    • Use New macro
    • declare SmartPointer<T> as Pointer
    • declare SmartPointer<const T> as ConstPointer
    • declare Self
    • declare Superclass
    • use TypeMacro
    • Have PrintSelf() method and print all the member variables
  • 100% code coverage (see dashboard)
  • Any information that is printed or displayed has to be legible to human eyes
  • Respect Coding Style as specified in the document Insight/Documentation/Style.pdf
  • Must pass the test of KWStyle
  • [Style] In the .txx files of template classes, the first 5 lines in the declaration of methods should be:
    • 1) template
    • 2) return type
    • 3) class name
    • 4) ::method name
    • 5) the opening bracket

For example: 

 template <  template_arguments >
 return_type
 classname< template_arguments >
 ::methodsName( method_arguments )
 {
   // indentation of two spaces
   // for the first line of code
   int k = 0;
 }

In many cases line (2) uses a trait from the templated class and therefore becomes: 

 typename classname< template_arguments >::trait

Running KWStyle

In a single file

  KWStyle -xml ITK.kws.xml itkQuadEdgeMeshTest1.cxx -v

In all the repository

  make StyleCheck


Const Correctness

  • Get method must be const
  • SetInput methods must take "const raw pointers" as input
  • GetOutput methods must give const raw pointers as output
  • All non-trivial types must be passed by reference (except for DataObjects, that must be passed by const raw pointers).
  • const_cast must only be used inside of the SetInput() method, to overcome the lack of const-correctness in the pipeline.
    • any other uses of const_cast must be scrutinized

Mini-Pipelines

Filters that implement a mini-pipeline with N internal filters should

  • Overload the method
  void EnlargeOutputRequestedRegion(DataObject *output)
  • Call ReleaseDataFlag() in the first N-1 filters of its mini-pipeline
  • For the filters that can run in-place, set them to do so.
  • Use the ProgressAccumulator in order to compose a progress report from each one of the internal filters
  • Use GrafOutput() in the following way at the end of the GenerateData() method
 caster->GraftOutput( outputImage );
 caster->Update();
 this->GraftOutput( caster->GetOutput() );


ITK: [Welcome | Site Map]


Progress Reporting

Filter should report

  • ProgressEvents or
  • IterationEvents

If they report ProgressEvents, they should use the ProgressReporter class as 

   ProgressReporter progress( this, threadId, outputRegionForThread.GetNumberOfPixels() );
   
   while( ... )
     {
     // do stuff
     progress.CompletedPixel();
     }

If the filter implements a mini-pipeline, then the progress reporting should be done using the ProgressAccumulator inside the GenerateData() method as: 

 ProgressAccumulator::Pointer progress = ProgressAccumulator::New();
 progress->SetMiniPipelineFilter(this);
 
 progress->RegisterInternalFilter( internalFilter01, fraction1 );
 progress->RegisterInternalFilter( internalFilter02, fraction2 );
 progress->RegisterInternalFilter( internalFilter03, fraction3 );
 progress->RegisterInternalFilter( internalFilter04, fraction4 );


where internalFilterX is each one of the filters in the mini-pipeline, and fractionX are number that add up to 1.0, and represent the fraction of time that each one of the filters will contribute to the total computation. This fraction is assigned as an estimation, and doesn't have to be perfect, just aim for the correct order of magnitude.


Multi-Threading

Filters that are multi-threaded, must implement the method: 

  void ThreadedGenerateData(
     const OutputImageRegionType & outputRegionForThread,
     int threadId)

and can (but do not have to) implement the methods 

   void BeforeThreadedGenerateData()
   void AfterThreadedGenerateData()

These two methods are intended to implement the preparation of variables that host computations on every thread, and to consolidate that information once the threads have completed their work.

The filter shouldn't implement the GenerateData() method, unless it needs anything different from what is provided in the default method in 

     ImageSource::GenerateData()

that essentially does: 

  void GenerateData()
  {
  this->AllocateOutputs();
  this->BeforeThreadedGenerateData();
  
  ThreadStruct str;
  str.Filter = this;
  
  this->GetMultiThreader()->SetNumberOfThreads( this->GetNumberOfThreads() );
  this->GetMultiThreader()->SetSingleMethod(this->ThreaderCallback, &str);
  
  this->GetMultiThreader()->SingleMethodExecute();
  
  this->AfterThreadedGenerateData();
  }

Any variables that need to be written into during the execution of the ThreadedGenerateData() method, must be put into an array, to provide an independent instance to every thread. This preparation can be done in the BeforeThreadedGenerateData() method, and the consolidation of values in the arrays can be done in the AfterThreadedGenerateData() method.

Example

For an example, look at the classes

  • itkStatisticsImageFilter
  • itkUnaryFunctorFilter


Member Variables

Filters should not use member variables to hold to their inputs or outputs.

The inputs and outputs should be maintained by the methods 

   SetInput(const InputImageType *input)
     {
     this->ProcessObject::SetNthInput( 0,const_cast< InputImageType * >( input ) );
     }



  GetInput( unsigned int idx ) 
    {
    return static_cast< const TInputImage * >( this->ProcessObject::GetInput(idx) );
    }



  GetOutput()
    {
    if ( this->GetNumberOfOutputs() < 1 )
      {
      return 0;
      }
    
    return static_cast< TOutputImage * >( this->ProcessObject::GetOutput(0) );
    }


In most cases, these methods are already provided by super classes. In particular by 

  * itk::ImageSource
  * itk::ImageToImageFilter
Retrieved from "https://public.kitware.com/Wiki/index.php?title=ITK/Release_4/Modularization/Code_Reviews/Checklist&oldid=37660"