Difference between revisions of "2009 Winter Project Week Compiler Warnings:Slicer3 Graffiti"

From NAMIC Wiki
Jump to: navigation, search
(New page: {| |thumb|320px|Return to [[2009_Winter_Project_Week|Project Week Main Page ]] |} __NOTOC__ ===Key Investigators=== * Bill Lorensen (His Basement) <div style="...)
 
 
(20 intermediate revisions by the same user not shown)
Line 1: Line 1:
 
{|
 
{|
 
|[[Image:NAMIC-SLC.jpg|thumb|320px|Return to [[2009_Winter_Project_Week|Project Week Main Page]] ]]
 
|[[Image:NAMIC-SLC.jpg|thumb|320px|Return to [[2009_Winter_Project_Week|Project Week Main Page]] ]]
 +
|[[Image:TippingPointBook.jpg|thumb|320px]]
 +
|[[Image:Slicer3Graffiti.png|thumb|320px]]
 
|}
 
|}
−
 
  
 
__NOTOC__
 
__NOTOC__
Line 19: Line 20:
 
In the ''Tipping Point'', a chapter on increasing crime in New York City in the 1990's describes how the trend was reversed. The city concentrated areas like cleaning graffiti from subway cars. The author claims that this and other seemingly trivial goals may have led to a reversal of the crime trend.
 
In the ''Tipping Point'', a chapter on increasing crime in New York City in the 1990's describes how the trend was reversed. The city concentrated areas like cleaning graffiti from subway cars. The author claims that this and other seemingly trivial goals may have led to a reversal of the crime trend.
  
−
This project hypothesizes that compilation warnings are the graffiti of software quality. A lack of attention to this seemingly trivial goal shows a lack of attention to the quality of software. This project will  
+
This project hypothesizes that compilation warnings are the graffiti of software quality. A lack of attention to this seemingly trivial goal shows a lack of attention to the overall quality of software. This project will reduce warnings to zero for a popular c++ compiler, GCC 4.3.
 +
 
 +
 
 
</div>
 
</div>
  
Line 26: Line 29:
 
<h1>Approach, Plan</h1>
 
<h1>Approach, Plan</h1>
  
−
This project will use the [http://en.wikipedia.org/wiki/DMAIC#DMAIC '''DMAIC'''] methodology of the [http://en.wikipedia.org/wiki/Six_Sigma ''Six Sigma management process''] to "Define", "Measure", "Analyze", "Improve" and "Control" compiler warnings in Slicer3.
+
This project will use the [http://en.wikipedia.org/wiki/DMAIC#DMAIC '''DMAIC'''] methodology of the [http://en.wikipedia.org/wiki/Six_Sigma ''Six Sigma management process''] to ''"Define"'', ''"Measure"'', ''"Analyze"'', ''"Improve"'' and ''"Control"'' compiler warnings in Slicer3. This project will concentrate on warnings produced by the GNU gcc 4.3 compiler. Similar approaches can be applied to other Slicer3 compilers.
 +
 
 +
The basic methodology (from Wikipedia) consists of the following five steps:
 +
*'''''D'''efine'' process goals that are consistent with customer demands and the NA-MIC's strategy.
 +
*'''''M'''easure'' key aspects of the current process and collect relevant data.
 +
*'''''A'''nalyze'' the data to verify cause-and-effect relationships. Determine what the relationships are, and attempt to ensure that all factors have been considered.
 +
*'''''I'''mprove'' or optimize the process.
 +
*'''''C'''ontrol'' to ensure that any deviations from target are corrected before they result in defects. Set up pilot runs to establish software quality, move on to production, set up control mechanisms and continuously monitor the process.
 +
 
 
</div>
 
</div>
  
Line 32: Line 43:
  
 
<h1>Progress</h1>
 
<h1>Progress</h1>
 +
*'''''D'''efine''
 +
The goal is to eliminate all compiler warnings produced by the GCC 4.3 c++ compiler and to implement measures to prevent warnings from entering the software.
 +
*'''''M'''easure''
 +
On December 1, 2008, a clean Slicer3 build using the gcc 4.3 compiler produced 523 warnings in 173 source files. The CXX_FLAGS
 +
''-Wall -Wshadow -Wcast-qual'' produced the following warnings:
 +
* 278: shadowed variables
 +
* 131: unused variable
 +
* 29: suggest parentheses around && within ||
 +
* 26: comparison between signed and unsigned integer expressions
 +
* 17:  deprecated conversion from string constant to ‘char*’
 +
* 14: defined but not used
 +
* 13: cast from type ‘const char*’ to type ‘char*’ casts away constness
 +
* 7: format expects type t argument has type ‘double’
 +
* 2: zero-length printf format string
  
−
 
+
*'''''A'''nalyze''
 +
A description of each warning, why it is important and how to eliminate it will be provided.
 +
*'''''I'''mprove''
 +
All Slicer3 source files with GCC warning defects will be repaired. This is a manual process. For this project, the xemacs ''compile'' module produced a ''*compilation*'' buffer. The ''compile-goto-next-error'' command loaded the defective file and positioned the cursor at the defective line of code. This facilitated the manual process. Still, the warning defect cleansing took approximately 16 hours of hand editing. Over '''200''' files were edited.
 +
*'''''C'''ontrol''
 +
The [http://www.cdash.org/CDash/index.php?project=Slicer3 Slicer3 dashboard] reports the warnings each day. Individuals will receive e-mails when they introduce warnings. Other more drastic measures will be investigated. For example, the GCC compiler flag ''-Werror'' can treat warnings as errors, thus blocking compilation of any files that have defects.
  
 
</div>
 
</div>
Line 40: Line 70:
  
 
</div>
 
</div>
 +
 +
==Status==
 +
All warnings from the gcc4.3 compiler on BillsBasement have been removed except for the optional IGT code. During project Week, I'll work with others that have different nightly build configurations.
 +
 +
===Additional Data===
 +
 +
Each ''shadowed variable'' warning produces 3 lines:
 +
<pre>
 +
Modules/Meshing/BuildingBlock/vtkKWMimxAssignBoundaryConditionsGroup.cxx: In member function ‘void tkKWMimxAssignBoundaryConditionsGroup::UpdateBCVisibilityList()’:
 +
Modules/Meshing/BuildingBlock/vtkKWMimxAssignBoundaryConditionsGroup.cxx:2382: warning: declaration of ‘stepNumber’ shadows a previous local
 +
Modules/Meshing/BuildingBlock/vtkKWMimxAssignBoundaryConditionsGroup.cxx:2363: warning: shadowed declaration is here
 +
</pre>
 +
 +
Each ''unused variable'' warning produces 1 line:
 +
<pre>
 +
Base/Logic/vtkSlicerApplicationLogic.cxx:1501: warning: unused variable ‘vnd’
 +
</pre>
 +
 +
Each ''suggest parentheses around && within ||'' warning produces 1 line:
 +
<pre>
 +
Base/GUI/vtkSlicerModelHierarchyWidget.cxx:980: warning: suggest parentheses around && within ||
 +
</pre>
 +
 +
Each ''comparison between signed and unsigned integer expressions'' warning produces 1 line:
 +
<pre>
 +
Base/Logic/vtkImageConnectivity.cxx:630: warning: comparison between signed and unsigned integer expressions
 +
</pre>
 +
 +
Each ''deprecated conversion from string constant to char*'' warning produces 1 line:
 +
<pre>
 +
Modules/QueryAtlas/vtkTextureFontManager.cxx:44: warning: deprecated conversion from string constant to ‘char*’
 +
</pre>
 +
 +
Each ''defined but not used'' warning produces 1 line:
 +
<pre>
 +
Modules/QueryAtlas/vtkTextureText.h:59: warning: ‘sAutoLeading’ defined but not used
 +
</pre>
 +
 +
Each ''cast from type const char* to type char* casts away constness'' produces 1 line:
 +
<pre>
 +
Modules/Tractography/Display/vtkSlicerFiberBundleLogic.cxx:131: warning: cast from type ‘const char*’ to type ‘char*’ casts away constness
 +
</pre>
 +
 +
Each ''format expects type , but argument has type'' produces 1 line:
 +
<pre>
 +
Modules/QdecModule/vtkGDFReader.cxx:389: warning: format ‘%g’ expects type ‘float*’, but argument 3 has type ‘double*’
 +
</pre>
 +
 +
Each ''zero-length printf format'' string produces 1 line:
 +
<pre>
 +
Base/GUI/vtkSlicerCacheAndDataIOManagerGUI.cxx:618: warning: zero-length printf format string
 +
</pre>
 +
== Dangerous Code Detected by Warnings ==
 +
 +
Compiler warnings are an annoyance. They stream by as you build your program. But tucked into those hundreds of lines of confusing messages, are a few that actually may affect the way your code runs.
 +
 +
{| border="1"
 +
! Warning
 +
! Code
 +
! Fixed Code
 +
! Who Cares?
 +
|-
 +
 +
|
 +
array subscript is above array bounds
 +
|
 +
  os << indent << "Cursor Color:            "
 +
    << this->CursorColor[0] << ","
 +
    << this->CursorColor['''2'''] << ","
 +
    << this->CursorColor['''3'''] << "\n";
 +
|
 +
  os << indent << "Cursor Color:            "
 +
    << this->CursorColor[0] << ","
 +
    << this->CursorColor['''1'''] << ","
 +
    << this->CursorColor['''2'''] << "\n";
 +
|
 +
Trying to debug your code? You add an fooObject->DebugON() and your code crashes. Sometimes...
 +
|-
 +
|
 +
array subscript is above array bounds
 +
|
 +
  double angle['''8'''];
 +
  ...
 +
  //corner 2
 +
  angle[6] = (vtkMath::Dot(b,c));
 +
  angle[7] = (vtkMath::Dot(f,b));
 +
  angle[8] = (vtkMath::Dot(f,c));
 +
|
 +
  double angle['''9'''];
 +
  ...
 +
  //corner 2
 +
  angle[6] = (vtkMath::Dot(b,c));
 +
  angle[7] = (vtkMath::Dot(f,b));
 +
  angle[8] = (vtkMath::Dot(f,c));
 +
|
 +
Runs on your system, crashes on your customers' systems
 +
|-
 +
|
 +
‘volume’ may be used uninitialized in this function
 +
|
 +
  double volume;
 +
  IteratorType it(image, image->GetLargestPossibleRegion());
 +
  for(it.GoToBegin();!it.IsAtEnd();++it){
 +
    if(it.Get())
 +
      volume++;
 +
  }
 +
|
 +
  double volume '''= 0.0''';
 +
  IteratorType it(image, image->GetLargestPossibleRegion());
 +
  for(it.GoToBegin();!it.IsAtEnd();++it){
 +
    if(it.Get())
 +
      volume++;
 +
  }
 +
|
 +
Every time you run it, the answers are different. Or runs in Debug, but not Release?
 +
|-
 +
|
 +
declaration of ‘BackgroundLayer’ shadows a member of 'this'
 +
|
 +
void vtkSlicerSliceLogic::SetBackgroundLayer(
 +
  vtkSlicerSliceLayerLogic *'''BackgroundLayer''')
 +
{
 +
  if (this->BackgroundLayer)
 +
    {
 +
    this->BackgroundLayer->SetAndObserveMRMLScene( NULL );
 +
    this->BackgroundLayer->Delete();
 +
    }
 +
  this->BackgroundLayer = '''BackgroundLayer''';
 +
...
 +
|
 +
void vtkSlicerSliceLogic::SetBackgroundLayer(
 +
  vtkSlicerSliceLayerLogic *'''backgroundLayer''')
 +
{
 +
  if (this->BackgroundLayer)
 +
    {
 +
    this->BackgroundLayer->SetAndObserveMRMLScene( NULL );
 +
    this->BackgroundLayer->Delete();
 +
    }
 +
  this->BackgroundLayer = '''backgroundLayer''';
 +
...
 +
|
 +
Leave out a this-> and you are in trouble.
 +
|-
 +
|
 +
statement has no effect
 +
|
 +
  //Enable/Disable navigation window
 +
  this->'''EnableDisableNavButton''';
 +
|
 +
  //Enable/Disable navigation window
 +
  this->'''EnableDisableNavButton = NULL''';
 +
|
 +
But, I disabled that button? Not...
 +
|-
 +
|
 +
comparison with string literal results in unspecified behaviour
 +
|
 +
  if (  '''this->GetUsername() == ""''' ||
 +
        '''this->GetPassword() == ""''')
 +
    {
 +
|
 +
  if (  '''strcmp(this->GetUsername(),"") == 0''' ||
 +
        '''strcmp(this->GetPassword(),"") == 0)'''
 +
    {
 +
|
 +
Works for me. Oh, not for you?
 +
|-
 +
|
 +
format ‘%g’ expects type ‘float*’, but argument 3 has type ‘double*’
 +
|
 +
sscanf(line, "%*s '''%g'''", &this->ResidualFWHM);
 +
|
 +
sscanf(line, "%*s '''%lg'''", &this->ResidualFWHM);
 +
|
 +
Sorry MAC.
 +
|-
 +
|
 +
comparison between signed and unsigned integer expressions
 +
|
 +
'''int''' pos = filename.find(".stl");
 +
|
 +
'''size_t''' pos = filename.find(".stl");
 +
|
 +
I just spent all of this money on a 64 bit system, now nothing works.
 +
|-
 +
|
 +
value computed is not used
 +
|
 +
(vtkMimxModPointWidget*)(Self->PointWidget->GetItemAsObject(i))
 +
  ->AddObserver(vtkCommand::InteractionEvent,Self->PWCallback);
 +
|
 +
'''('''(vtkMimxModPointWidget*)(Self->PointWidget->GetItemAsObject(i))''')'''
 +
  ->AddObserver(vtkCommand::InteractionEvent,Self->PWCallback);
 +
|
 +
A tricky one. Can you find it? The compiler did.
 +
|-
 +
|
 +
format '%d' expects type 'int', but argument 3 has type 'size_t'
 +
|
 +
fprintf(stderr,
 +
  "ERROR: QdecGlmDesign contrast size '''%d''' != %d\n",
 +
  contrast.size(), nreg);
 +
|
 +
fprintf(stderr,
 +
  "ERROR: QdecGlmDesign contrast size '''%ul''' != %d\n",
 +
  '''(unsigned long)''' contrast.size(), nreg);
 +
|
 +
Long live 32 bits!
 +
|-
 +
|
 +
comparison is always true due to limited range of data type
 +
|
 +
'''unsigned int''' i;
 +
std::string tags;
 +
i = '''(unsigned int)''' tags.find("=");
 +
if ( i != std::string::npos)
 +
|
 +
'''std::string::size_type''' i;
 +
std::string tags;
 +
i = tags.find("=");
 +
if ( i != std::string::npos)
 +
|
 +
stl is your friend.
 +
|-
 +
|}

Latest revision as of 19:32, 5 January 2009

Home < 2009 Winter Project Week Compiler Warnings:Slicer3 Graffiti
Return to Project Week Main Page
TippingPointBook.jpg
Slicer3Graffiti.png



Key Investigators

  • Bill Lorensen (His Basement)

Objective

The book, The Tipping Point, analyses trends and how they take hold. As Slicer3 grows and the number of contributors increases, there needs to be tighter controls on the quality of the system. The larger the code base, number of platforms and developers, the more need there is for attention to quality.

In the Tipping Point, a chapter on increasing crime in New York City in the 1990's describes how the trend was reversed. The city concentrated areas like cleaning graffiti from subway cars. The author claims that this and other seemingly trivial goals may have led to a reversal of the crime trend.

This project hypothesizes that compilation warnings are the graffiti of software quality. A lack of attention to this seemingly trivial goal shows a lack of attention to the overall quality of software. This project will reduce warnings to zero for a popular c++ compiler, GCC 4.3.


Approach, Plan

This project will use the DMAIC methodology of the Six Sigma management process to "Define", "Measure", "Analyze", "Improve" and "Control" compiler warnings in Slicer3. This project will concentrate on warnings produced by the GNU gcc 4.3 compiler. Similar approaches can be applied to other Slicer3 compilers.

The basic methodology (from Wikipedia) consists of the following five steps:

  • Define process goals that are consistent with customer demands and the NA-MIC's strategy.
  • Measure key aspects of the current process and collect relevant data.
  • Analyze the data to verify cause-and-effect relationships. Determine what the relationships are, and attempt to ensure that all factors have been considered.
  • Improve or optimize the process.
  • Control to ensure that any deviations from target are corrected before they result in defects. Set up pilot runs to establish software quality, move on to production, set up control mechanisms and continuously monitor the process.

Progress

  • Define

The goal is to eliminate all compiler warnings produced by the GCC 4.3 c++ compiler and to implement measures to prevent warnings from entering the software.

  • Measure

On December 1, 2008, a clean Slicer3 build using the gcc 4.3 compiler produced 523 warnings in 173 source files. The CXX_FLAGS -Wall -Wshadow -Wcast-qual produced the following warnings:

  • 278: shadowed variables
  • 131: unused variable
  • 29: suggest parentheses around && within ||
  • 26: comparison between signed and unsigned integer expressions
  • 17: deprecated conversion from string constant to ‘char*’
  • 14: defined but not used
  • 13: cast from type ‘const char*’ to type ‘char*’ casts away constness
  • 7: format expects type t argument has type ‘double’
  • 2: zero-length printf format string
  • Analyze

A description of each warning, why it is important and how to eliminate it will be provided.

  • Improve

All Slicer3 source files with GCC warning defects will be repaired. This is a manual process. For this project, the xemacs compile module produced a *compilation* buffer. The compile-goto-next-error command loaded the defective file and positioned the cursor at the defective line of code. This facilitated the manual process. Still, the warning defect cleansing took approximately 16 hours of hand editing. Over 200 files were edited.

  • Control

The Slicer3 dashboard reports the warnings each day. Individuals will receive e-mails when they introduce warnings. Other more drastic measures will be investigated. For example, the GCC compiler flag -Werror can treat warnings as errors, thus blocking compilation of any files that have defects.


Status

All warnings from the gcc4.3 compiler on BillsBasement have been removed except for the optional IGT code. During project Week, I'll work with others that have different nightly build configurations.

Additional Data

Each shadowed variable warning produces 3 lines: 

Modules/Meshing/BuildingBlock/vtkKWMimxAssignBoundaryConditionsGroup.cxx: In member function ‘void tkKWMimxAssignBoundaryConditionsGroup::UpdateBCVisibilityList()’:
Modules/Meshing/BuildingBlock/vtkKWMimxAssignBoundaryConditionsGroup.cxx:2382: warning: declaration of ‘stepNumber’ shadows a previous local
Modules/Meshing/BuildingBlock/vtkKWMimxAssignBoundaryConditionsGroup.cxx:2363: warning: shadowed declaration is here

Each unused variable warning produces 1 line: 

Base/Logic/vtkSlicerApplicationLogic.cxx:1501: warning: unused variable ‘vnd’

Each suggest parentheses around && within || warning produces 1 line: 

Base/GUI/vtkSlicerModelHierarchyWidget.cxx:980: warning: suggest parentheses around && within ||

Each comparison between signed and unsigned integer expressions warning produces 1 line: 

Base/Logic/vtkImageConnectivity.cxx:630: warning: comparison between signed and unsigned integer expressions

Each deprecated conversion from string constant to char* warning produces 1 line: 

Modules/QueryAtlas/vtkTextureFontManager.cxx:44: warning: deprecated conversion from string constant to ‘char*’

Each defined but not used warning produces 1 line: 

Modules/QueryAtlas/vtkTextureText.h:59: warning: ‘sAutoLeading’ defined but not used

Each cast from type const char* to type char* casts away constness produces 1 line: 

Modules/Tractography/Display/vtkSlicerFiberBundleLogic.cxx:131: warning: cast from type ‘const char*’ to type ‘char*’ casts away constness

Each format expects type , but argument has type produces 1 line: 

Modules/QdecModule/vtkGDFReader.cxx:389: warning: format ‘%g’ expects type ‘float*’, but argument 3 has type ‘double*’

Each zero-length printf format string produces 1 line: 

Base/GUI/vtkSlicerCacheAndDataIOManagerGUI.cxx:618: warning: zero-length printf format string

Dangerous Code Detected by Warnings

Compiler warnings are an annoyance. They stream by as you build your program. But tucked into those hundreds of lines of confusing messages, are a few that actually may affect the way your code runs.

Warning Code Fixed Code Who Cares?

array subscript is above array bounds 

 os << indent << "Cursor Color:             " 
   << this->CursorColor[0] << ","
   << this->CursorColor[2] << ","
   << this->CursorColor[3] << "\n";

 os << indent << "Cursor Color:             " 
   << this->CursorColor[0] << ","
   << this->CursorColor[1] << ","
   << this->CursorColor[2] << "\n";

Trying to debug your code? You add an fooObject->DebugON() and your code crashes. Sometimes...

array subscript is above array bounds 

 double angle[8];
 ...
 //corner 2 
 angle[6] = (vtkMath::Dot(b,c));
 angle[7] = (vtkMath::Dot(f,b));
 angle[8] = (vtkMath::Dot(f,c));

 double angle[9];
 ...
 //corner 2 
 angle[6] = (vtkMath::Dot(b,c));
 angle[7] = (vtkMath::Dot(f,b));
 angle[8] = (vtkMath::Dot(f,c));

Runs on your system, crashes on your customers' systems

‘volume’ may be used uninitialized in this function 

 double volume;
 IteratorType it(image, image->GetLargestPossibleRegion());
 for(it.GoToBegin();!it.IsAtEnd();++it){
   if(it.Get())
     volume++;
 }

 double volume = 0.0;
 IteratorType it(image, image->GetLargestPossibleRegion());
 for(it.GoToBegin();!it.IsAtEnd();++it){
   if(it.Get())
     volume++;
 }

Every time you run it, the answers are different. Or runs in Debug, but not Release?

declaration of ‘BackgroundLayer’ shadows a member of 'this' 

void vtkSlicerSliceLogic::SetBackgroundLayer(
  vtkSlicerSliceLayerLogic *BackgroundLayer)
{
 if (this->BackgroundLayer)
   {
   this->BackgroundLayer->SetAndObserveMRMLScene( NULL );
   this->BackgroundLayer->Delete();
   }
 this->BackgroundLayer = BackgroundLayer;
...

void vtkSlicerSliceLogic::SetBackgroundLayer(
  vtkSlicerSliceLayerLogic *backgroundLayer)
{
 if (this->BackgroundLayer)
   {
   this->BackgroundLayer->SetAndObserveMRMLScene( NULL );
   this->BackgroundLayer->Delete();
   }
 this->BackgroundLayer = backgroundLayer;
...

Leave out a this-> and you are in trouble.

statement has no effect 

 //Enable/Disable navigation window
 this->EnableDisableNavButton;

 //Enable/Disable navigation window
 this->EnableDisableNavButton = NULL;

But, I disabled that button? Not...

comparison with string literal results in unspecified behaviour 

 if (  this->GetUsername() == "" ||
       this->GetPassword() == "")
   {

 if (  strcmp(this->GetUsername(),"") == 0 ||
       strcmp(this->GetPassword(),"") == 0)
   {

Works for me. Oh, not for you?

format ‘%g’ expects type ‘float*’, but argument 3 has type ‘double*’ 

sscanf(line, "%*s %g", &this->ResidualFWHM);

sscanf(line, "%*s %lg", &this->ResidualFWHM);

Sorry MAC.

comparison between signed and unsigned integer expressions 

int pos = filename.find(".stl");

size_t pos = filename.find(".stl");

I just spent all of this money on a 64 bit system, now nothing works.

value computed is not used 

(vtkMimxModPointWidget*)(Self->PointWidget->GetItemAsObject(i))
 ->AddObserver(vtkCommand::InteractionEvent,Self->PWCallback);

((vtkMimxModPointWidget*)(Self->PointWidget->GetItemAsObject(i)))
 ->AddObserver(vtkCommand::InteractionEvent,Self->PWCallback);

A tricky one. Can you find it? The compiler did.

format '%d' expects type 'int', but argument 3 has type 'size_t' 

fprintf(stderr,
 "ERROR: QdecGlmDesign contrast size %d != %d\n",
 contrast.size(), nreg);

fprintf(stderr,
 "ERROR: QdecGlmDesign contrast size %ul != %d\n",
 (unsigned long) contrast.size(), nreg);

Long live 32 bits!

comparison is always true due to limited range of data type 

unsigned int i;
std::string tags;
i = (unsigned int) tags.find("=");
if ( i != std::string::npos)

std::string::size_type i;
std::string tags;
i = tags.find("=");
if ( i != std::string::npos)

stl is your friend.

Retrieved from "https://www.na-mic.org/w/index.php?title=2009_Winter_Project_Week_Compiler_Warnings:Slicer3_Graffiti&oldid=34298"