2009 Winter Project Week Compiler Warnings:Slicer3 Graffiti

Return to Project Week Main Page

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.

• 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.

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

Warning	Code	Fixed Code
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";
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));
‘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++; }
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; ...
statement has no effect	//Enable/Disable navigation window this->EnableDisableNavButton;	//Enable/Disable navigation window this->EnableDisableNavButton = NULL;
comparison with string literal results in unspecified behaviour	if ( this->GetUsername() == "" || this->GetPassword() == "") {	if ( strcmp(this->GetUsername(),"") == 0 || strcmp(this->GetPassword(),"") == 0) {
format ‘%g’ expects type ‘float*’, but argument 3 has type ‘double*’	sscanf(line, "%*s %g", &this->ResidualFWHM);	sscanf(line, "%*s %lg", &this->ResidualFWHM);
comparison between signed and unsigned integer expressions	int pos = filename.find(".stl");	size_t pos = filename.find(".stl");
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);
format '%d' expects type 'int', but argument 3 has type 'size_t'	fprintf(stderr, "ERROR: QdecGlmDesign::GenerateContrasts: contrast size %d != %d\n", contrast.size(), nreg);	fprintf(stderr, "ERROR: QdecGlmDesign::GenerateContrasts: contrast size %ul != %d\n", (unsigned long) contrast.size(), nreg);
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)