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