<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0 Transitional//EN"
"http://www.w3.org/TR/REC-html40/loose.dtd">
<html>
<head>
<title>VTK 3.1.2 GCC 2.91.66 Warnings</title>
<meta http-equiv="Content-Type" content="text/html">
</head>
<body>
<h1>VTK 3.1.2 GCC 2.91.66 Warnings</h1>
<h2>1 Experiment</h2>
<h3>1.1 Introduction</h3>
<p>To improve the usability of <em>VTK</em> I decided to investigate the
posibility of making it compile warning free.</p>
<h3>1.2 Procedure</h3>
<p>I ran the default <code>./configure</code> on a <em>VTK 3.1.2</em> install.
Then I edited the <code>user.make</code> file to pass the following flags to
<em>GCC 2.91.66</em>, <code>-W -Wall -Weffc++</code>. Finally, I ran
<code>make > report 2>&1</code> to record the output.</p>
<h3>1.3 Results</h3>
<p>The following is a list of the warnings generated, sorted by frequency.
Warnings generated by the standard library (yes, that's right!) are not
included. Each unique warning is counted only once. There were 4621 warnings
generated in total. 2713 were in header files; 1908 were in cxx files.</p>
<ol>
<li>'class_name::member_name' should be initialized in the member
initialization list (count: 3574)</li>
<li>'operator=' should return a reference to '*this' (count: 500)</li>
<li>base class 'class class_name' should be explicitly initialized in the
copy constructor (count: 492)</li>
<li>suggest parentheses around assignment used as truth value (count:
28)</li>
<li>'class class_name' has pointer data members but does not override
'class_name(const class_name&)' or 'operator=(const class_name&)' (count:
16)</li>
<li>unused variable 'variable_type variable_name' (count: 7)</li>
<li>member initializers for 'variable_type variable_name' and 'variable_type
variable_name' will be re-ordered to match declaration order (count:
3)</li>
<li>unused parameter 'parameter_type parameter_name' (count: 2)</li>
<li>'class class_name' has virtual functions but non-virtual destructor
(count: 1)</li>
</ol>
<p>Note: These numbers do not add up properly due to counting mistakes I made
caused by the fact that <em>GCC</em> usually spreads messages over several
lines, which made it hard to use <code>wc</code> effectively.</p>
<h3>1.4 Conclusions</h3>
<p>If scripts could be written to resolve the first three classes of warning,
then the remaining 57 warnings could easily be resolved by hand.</p>
<p>In fact, the first two warnings on the list were generated by the
<code>-Weffc++</code> switch. That means that one, possibly simple, script and
a manual cleaning of the last 57 warnings would at least allow <code>-W
-Wall</code>, which is what you usually want anyway.</p>
<h2>2 Follow-up</h2>
<h3>2.1 Patch File</h3>
<p>A patch file, <code>vtk312-warnings.patch</code> (375k), has been
created which cleans up the approximately 450 warnings described by the last 7
items of the list above. This means that version 3.1.2 of the <em>VTK</em>
library compiles warning free with <code>-W -Wall</code> switches under
<e>GCC. To apply the patch follow these steps:</p>
<ol>
<li><code>tar xvf vtk312Unix.tar</code></li>
<li><code>cd vtk31</code></li>
<li>copy <code>vtk312-warnings.patch</code> into this directory</li>
<li><code>patch -p1 < vtk312-warnings.patch</code></li>
<li>add <code>-W -Wall</code> to <code>USER_CXXFLAGS</code> in
<code>user.make</code> (so you can see the benefit)</li>
<li><code>./configure</code></li>
<li><code>make</code></li>
</ol>
<p>Obviously, if you've got an existing install, especially one with your own
patches, you'll want to follow a slightly different procedure. The patch file
was created by <code>diff -r -C2 vtk31-old/ vtk31-new/</code>, then I removed
a few irrelevant things manually.</p>
<h3>2.2 Change Summary</h3>
<p>In no case did a warning appear to point out a serious error. This was not
unexpected. Afterall, they are <strong>just</strong> warnings and <em>VTK</em>
is extensively tested. The warnings were dealt with as follows:</p>
<ul>
<li><p><em>base class 'class class_name' should should be explicitly
initialized in the copy constructor</em></p>
<p>All but 3 instances of this warning were removed by a simple 30 line
<em>Perl</em> script. The script searched for empty copy constructor
definitions of the form <code>class_name(const class_name&) {};</code>
and replaced them with declarations. <em>VTK</em> was using the empty
definitions to prevent the compiler from generating default public copy
constructors. A discussion of this problem can be found in Item 27 of
"Effective C++". Suffice it to say, declarations are sufficient.</p>
<p><em>(Note: The same script could also be used as a base to attack the
<code>operator=</code> warning described above.)</em></p>
<p>The last 3 copy constructor warnings were resolved in the same way, by
deleting the definition, but more work was involved. Because the copy
constructors contained code and were documented as shallow copy
constructors, I had to ensure they were dead code before I could delete
them. All the copy constructors I looked at were <code>protected</code> so
they can only be called by derived classes, this means it is unlkely that
client code would contain copy constructor calls. Even if client code did
contain calls, it is unlikely that they work since the base class copy
constructors, for <code>vtkObject</code> for example, were incomplete. To
ensure they were not used in <em>VTK</em> I had to check for explict calls
and pass-by-value. Pass-by-value was a deadend, I found a few explict
calls. These turned out to be from other copy constructor so I repeated
the checks for these. At the end of the day I concluded the code was dead
and ripped it out.</p>
<p><em>(Note: To check for pass-by-value and explicit calls I used the
following regex <code>class_name(([^&*:.{_a-zA-Z0-9 \t])|([
\t]*[(]))</code>.)</em></p>
</li>
<li><p><em>suggest parentheses around assignment used as thruth
value</em></p>
<p>Parentheses were added and the result was compared to 0. For
example,</p>
<pre><code>if (pPointer=GetPointer())</code></pre>
<p>became</p>
<pre <code> if (0 != (pPointer=GetPoiner()))
</pre>
</li>
<li><p><em>'class class_name' has pointer data members but does not override
'class_name(const class_name&)' or 'operator=(const class_name&)'</em></p>
<p>Copy constructors and assignment operators were declared in the class'
<code>private</code> section, but were not defined. This prevents the
compiler from generating them, see Item 27 of "Effective C++".</p>
</li>
<li><p><em>unused variable 'variable_type variable_name'</em></p>
<p>The unused variable was commented out. They were not simply deleted
because they were always initialized with the return value of a method
call. I was worried that the method call might have a desirable
side-effect, so it was left. For example,</p>
<pre><code>int num=pIn->GetSize();</code></pre>
<p>became</p>
<pre><code>/*int num=*/pIn->GetSize();</code></pre>
</li>
<li><p><em>member initializers for 'variable_type variable_name' and
'variable_type variable_name' will be re-ordered to match declaration
order</em></p>
<p>The member initializer list was re-ordered to match declaration
order.</p>
</li>
<li><p><em>unused parameter 'parameter_type parameter_name'</em></p>
<p>The variable name was commented out. For example,</p>
<pre><code>void function( int used, int unused )</code></pre>
became
<pre><code>void function( int used, int /*unused*/ )</code></pre>
</li>
<li><p><em>'class class_name' has virtual functions but non-virtual
destructor</em></p>
<p>The destructor was declared <code>virtual</code>.</p>
</li>
</ul>
<h3>2.3 Conclusion</h3>
<p>Warnings sometimes have a tendency to be 'squishy'. That is, removing a
warning on one platform, sometimes introduces warnings on another platform.
Unfortunately, I can not test other platforms.</p>
<p>Worse, code changes of any sort can sometimes introduce bugs. I've been
very careful to document my changes and the reasons for them, and to ensure
that the changes I actually made match the documentation. However, there is
always the possibility that I broke something somewhere. Hopefully, I didn't
break anything since the point of this exercise was to make the code harder to
break by increasing the warning level of the compiler.</p>
<p>-- end --</p>
</body>
</html>