Difference between revisions of "Writing warning-free code"

From Apache OpenOffice Wiki
Jump to: navigation, search
m (When all else fails)
 
(45 intermediate revisions by 12 users not shown)
Line 1: Line 1:
On CWS [http://eis.services.openoffice.org/EIS2/servlet/cws.showCWS?Id=2903&Path=SRC680%2Fwarnings01 warnings01] we are currently on the way towards a completely warning-free OOo C/C++ code base.  And once we have reached that, we want to keep the code clean!
+
We are currently on the way towards a completely warning-free OOo C/C++ code base.  And once we have reached that, we want to keep the code clean!
  
On that CWS, you currently need to build with the build switch <code>werror=1</code> to turn compiler warnings into errors, as for example in
+
We reached a first milestone by integrating CWS [http://eis.services.openoffice.org/EIS2/servlet/cws.showCWS?Id=2903&Path=SRC680%2Fwarnings01 warnings01] into SRC680m173.  Since then, and at least on the <code>unxlngi6</code>, <code>unxsoli4</code>, <code>unxsols4</code>, and <code>wntmsci10</code> platforms, the C/C++ compilers are used with rather aggressive warning levels, and in most CVS modules warnings are treated as errors that break the build.  Some CVS modules have not yet been made warning-free.
<blockquote><code>build werror=1</code></blockquote>
+
There is also a build switch <code>wall=1</code> to switch on as many compiler warnings as usefull, but its effect is already switched on per default on the four Hamburg-relevant platforms <code>unxlngi6</code>, <code>unxsoli4</code>, <code>unxsols4</code>, and <code>wntmsci10</code>.  In the future, the behaviour triggered by those switches may become default behaviour (on all platforms), so that the switches may no longer be necessary.
+
  
If your job is to clean up a given CVS module on that CWS, here are some practical guidelines:
+
==What changed?==
* Work directly on the CWS (<code>/cws/cws03/warnings01</code> in Hamburg).
+
 
* Make sure your module builds and delivers successfully with <code>build werror=1 && deliver</code>.<br/>Note that you can also set an environment variable "werror" to "1" (or "true" or whatever) - then a simply <code>build</code> will do.
+
Since SRC680m173, any new or modified C/C++ code that produces warnings will break the build.  (Unless, of course, you fail to pass <code>--enable-werror</code> to <code>configure</code>.)  Ideally, to be sure that integrating a CWS will not break the build, the CWS should now be built on every warning-free platform (i.e., at least <code>unxlngi6</code>, <code>unxsoli4</code>, <code>unxsols4</code>, and <code>wntmsci10</code>), with all supported compiler versions, in both product and non-product variants, and with and without the <code>debug=x</code> build switch before nominating it for integration.  Practically, a reasonable subset of these will have to suffice.
 +
 
 +
For <code>unxlngi6</code>, we concentrated on GCC&nbsp;3.4.1, GCC 4.0.2/4.0.3, and GCC&nbsp;4.1.1, while other GCC versions probably produce slightly different warnings (that could break the build now on those GCC versions).
 +
 
 +
Porters are encouraged to make additional platforms warning-free.  Since most ports are using GCC, there should not be too many surprises.  Any necessary changes to the platform-specific <code>.mk</code> file in <code>solenv/inc</code> can be modelled after <code>solenv/inc/unxlngi6.mk</code>.  In short, you need a bunch of <code>CFLAGSxxx</code> (see [http://www.openoffice.org/servlets/ReadMsg?list=interface-announce&msgNo=758 interface-announce C/C++ compiler warnings]) and <code>MODULES_WITH_WARNINGS</code> or <code>COMPILER_WARN_ERRORS</code> (see [http://www.openoffice.org/servlets/ReadMsg?list=interface-announce&msgNo=942 interface-announce MODULES_WITH_WARNINGS]).
 +
 
 +
The <code>wall=x</code> build switch no longer has any effect on the warning-free platforms.  Once all platforms are warning-free, we can remove it completely.
 +
 
 +
==The missing modules==
 +
 
 +
If your job is to clean up a given CVS module, here are some practical guidelines:
 +
* Remove the module from the <code>MODULES_WITH_WARNINGS</code> list in the platform-specific <code>.mk</code> files in <code>solenv/inc</code>.
 +
* Make sure your module builds and delivers successfully with <code>build && deliver</code> (and all the variants mentioned above).
 
* If your module contains external code which we do not want to clean up, set <code>EXTERNAL_WARNINGS_NOT_ERRORS := TRUE</code> at the beginning of the corresponding <code>makefile.mk</code>, so that warnings do not lead to errors there.
 
* If your module contains external code which we do not want to clean up, set <code>EXTERNAL_WARNINGS_NOT_ERRORS := TRUE</code> at the beginning of the corresponding <code>makefile.mk</code>, so that warnings do not lead to errors there.
* In any case, for any header file delivered from your module, make sure that <code>testhxx</code> on the delivered header file is successful.
+
* In any case, for any header file delivered from your module, make sure that <code>testhxx</code> on the delivered header file is successful. (Note that <code>testhxx</code> only works for headers delivered to the solver, not for local headers within a CVS module, as it calls the compiler with only a fixed set of include directories.)
* If you encounter a warning that cannot reasonably be worked around, discuss that problem on <code>dev@openoffice.org</code>.  One outcome of that discussion can be that we globally disable that particular warning.  To see which warnings are already globally disabled on a given platform, look at the settings for <code>CFLAGSWARNCC</code>, <code>CFLAGSWARNCXX</code>, <code>CFLAGSWALLCC</code>, and/or <code>CFLAGSWALLCXX</code> in the platform-specific <code>.mk</code> file in <code>solenv/inc</code> (<code>unxlngi6.mk</code>, <code>unxsoli4.mk</code>, <code>unxsols4.mk</code>, and the <code>"$(COMEX)"==10</code> branch of <code>wnt.mk</code>).
+
* If you encounter a warning that cannot reasonably be worked around, discuss that problem on <code>dev@openoffice.org</code>.  One outcome of that discussion can be that we globally disable that particular warning.  To see which warnings are already globally disabled on a given platform, look at the settings for <code>CFLAGSWARNCC</code>, <code>CFLAGSWARNCXX</code>, <code>CFLAGSWALLCC</code>, and/or <code>CFLAGSWALLCXX</code> in the platform-specific <code>.mk</code> file in <code>solenv/inc</code> (<code>unxlngi6.mk</code>, <code>unxsoli4.mk</code>, <code>unxsols4.mk</code>, and <code>wntmsci10.mk</code>).
* Commit your changes (do not forget to add your module to the CWS before making any changes).<br/>**Attention**If you do changes to your module *before* doing a <code>cwsadd</code>, then <code>cwsadd</code> will throw away all your changes!
+
 
* In a first step, we concentrate on cleaning up the code for <code>unxlngi6.pro</code>.  In a second step, the other Hamburg platforms (<code>unxlngi6</code>, <code>unxsoli4.pro</code>, <code>unxsols4</code>, <code>unxsols4.pro</code>, <code>wntmsci10</code>, and <code>wntmsci10.pro</code>) will follow.
+
See the current [[Warning-free_code_status|warning-free code status]].
* Anounce in a mail to all people involved in the CWS that your module is cleaned up for <code>unxlngi6.pro</code> (or even more platforms).
+
 
* <code>/tausch/cl/warningfreecode.csv</code> in Hamburg represents the current status.  That document is updated by Stephan Bergmann from the anouncement mails; do not modify it yourself.
+
==Tips for Warning-free code==
  
 
The following is a list of issues you might come across when making existing
 
The following is a list of issues you might come across when making existing
Line 22: Line 32:
  
 
Sometimes, a cast between integral types is needed to avoid a warning.  In such a case, use <code>sal::static_int_cast</code>&nbsp;(C++) or <code>SAL_INT_CAST</code>&nbsp;(C) to make that cast stick out&mdash;it will need to be adapted when the type of any of the involved expressions changes later on:
 
Sometimes, a cast between integral types is needed to avoid a warning.  In such a case, use <code>sal::static_int_cast</code>&nbsp;(C++) or <code>SAL_INT_CAST</code>&nbsp;(C) to make that cast stick out&mdash;it will need to be adapted when the type of any of the involved expressions changes later on:
<code>[cpp]
+
<syntaxhighlight lang="cpp">
 
sal_Int32 n;
 
sal_Int32 n;
 
std::vector<char> v;
 
std::vector<char> v;
 
if (n >= 0 && sal::static_int_cast<sal_uInt32>(n) == v.size()) ...
 
if (n >= 0 && sal::static_int_cast<sal_uInt32>(n) == v.size()) ...
</code>
+
</syntaxhighlight>
 +
 
 +
Note that you might get the warning C4244 (possible loss of data) on Windows also for the following situation:
 +
<syntaxhighlight lang="cpp">
 +
short a = 0, b = 0;
 +
a += b;  // warning C4244 on Windows
 +
</syntaxhighlight>
 +
This is because <code>a</code> and <code>b</code> are converted to <code>int</code>, then added and the result is then again cast back to <code>short</code>. This is especially nasty, as it also happens when you use <code>xub_StrLen</code>, but only if it is a typedf to <code>USHORT</code> and not to <code>sal_uInt32</code>, i.e. only as long as <code>STRING32</code> is not defined in solar.h.
 +
The warning does not occur when you use <code>operator+</code> instead of <code>operator+=</code>.
 +
<syntaxhighlight lang="cpp">
 +
short a = 0, b = 0;
 +
a = a + b;
 +
</syntaxhighlight>
 +
 
 +
===Invalid enum case values===
 +
 
 +
Code like
 +
<syntaxhighlight lang="cpp">
 +
enum E { A=1, B=2, C=4 } e;
 +
switch (e) {
 +
case A: ...
 +
case B|C: ...
 +
}
 +
</syntaxhighlight>
 +
will cause <code>wntmsci11</code> to complain with <code>warning C4063: case '6' is not a valid value for switch of enum 'E'</code>. The cause of the warning is that <code>E</code> is not meant be an enumerated type, but a bit field type. <code>enum</code> is commonly used in C and C++ to initialize constants; in this case, constants for individual bits.
 +
 
 +
A better way to write this would be to typedef <code>E</code> to be an unsigned int (a better type for bit fields); and then define the constants for the individual bits using an anonymous <code>enum</code>. (A downside is that the documentation for the type <code>E</code> will not contain the constants the user needs to use.)
 +
<syntaxhighlight lang="cpp">
 +
typedef unsigned int E;
 +
E e;
 +
enum { A=1, B=2, C=4 };
 +
switch (e) {
 +
case A: ...
 +
case B|C: ...
 +
}
 +
</syntaxhighlight>
 +
 
 +
If you do not have access to the definition of the type <code>E</code>, it can still be worked around by promoting <code>e</code> to its underlying integral type:
 +
<syntaxhighlight lang="cpp">
 +
...
 +
switch (+e) {
 +
...
 +
</syntaxhighlight>
  
 
===Casting between pointer and integer types===
 
===Casting between pointer and integer types===
Line 33: Line 85:
  
 
* From pointer-to-(possibly-cv-qualified-)void-or-object type&nbsp;<code>P</code> to unsigned integral type&nbsp;<code>U</code>:
 
* From pointer-to-(possibly-cv-qualified-)void-or-object type&nbsp;<code>P</code> to unsigned integral type&nbsp;<code>U</code>:
<code>[cpp]
+
<syntaxhighlight lang="cpp">
 
P p;
 
P p;
 
U u = sal::static_int_cast<U>(reinterpret_cast<sal_uIntPtr>(p));
 
U u = sal::static_int_cast<U>(reinterpret_cast<sal_uIntPtr>(p));
</code>
+
</syntaxhighlight>
  
 
* From unsigned integral type&nbsp;<code>U</code> to pointer-to-(possibly-cv-qualified-)void-or-object type&nbsp;<code>P</code>:
 
* From unsigned integral type&nbsp;<code>U</code> to pointer-to-(possibly-cv-qualified-)void-or-object type&nbsp;<code>P</code>:
<code>[cpp]
+
<syntaxhighlight lang="cpp">
 
U u;
 
U u;
 
P p = reinterpret_cast<P>(sal::static_int_cast<sal_uIntPtr>(u));
 
P p = reinterpret_cast<P>(sal::static_int_cast<sal_uIntPtr>(u));
</code>
+
</syntaxhighlight>
  
 
* From pointer-to-(possibly-cv-qualified-)void-or-object type&nbsp;<code>P</code> to signed integral type&nbsp;<code>S</code>:
 
* From pointer-to-(possibly-cv-qualified-)void-or-object type&nbsp;<code>P</code> to signed integral type&nbsp;<code>S</code>:
<code>[cpp]
+
<syntaxhighlight lang="cpp">
 
P p;
 
P p;
 
S s = sal::static_int_cast<S>(reinterpret_cast<sal_IntPtr>(p));
 
S s = sal::static_int_cast<S>(reinterpret_cast<sal_IntPtr>(p));
</code>
+
</syntaxhighlight>
  
 
* From signed integral type&nbsp;<code>S</code> to pointer-to-(possibly-cv-qualified-)void-or-object type&nbsp;<code>P</code>:
 
* From signed integral type&nbsp;<code>S</code> to pointer-to-(possibly-cv-qualified-)void-or-object type&nbsp;<code>P</code>:
<code>[cpp]
+
<syntaxhighlight lang="cpp">
 
S s;
 
S s;
 
P p = reinterpret_cast<P>(sal::static_int_cast<sal_IntPtr>(s));
 
P p = reinterpret_cast<P>(sal::static_int_cast<sal_IntPtr>(s));
</code>
+
</syntaxhighlight>
  
 
====C====
 
====C====
  
 
* From pointer-to-(possibly-qualified-)void-or-object type&nbsp;<code>P</code> to unsigned integer type&nbsp;<code>U</code>:
 
* From pointer-to-(possibly-qualified-)void-or-object type&nbsp;<code>P</code> to unsigned integer type&nbsp;<code>U</code>:
<code>[c]
+
<syntaxhighlight lang="c">
 
P p;
 
P p;
 
U u = SAL_INT_CAST(U, (sal_uIntPtr) p);
 
U u = SAL_INT_CAST(U, (sal_uIntPtr) p);
</code>
+
</syntaxhighlight>
  
 
* From unsigned integer type&nbsp;<code>U</code> to pointer-to-(possibly-qualified-)void-or-object type&nbsp;<code>P</code>:
 
* From unsigned integer type&nbsp;<code>U</code> to pointer-to-(possibly-qualified-)void-or-object type&nbsp;<code>P</code>:
<code>[c]
+
<syntaxhighlight lang="c">
 
U u;
 
U u;
 
P p = (P) SAL_INT_CAST(sal_uIntPtr, u);
 
P p = (P) SAL_INT_CAST(sal_uIntPtr, u);
</code>
+
</syntaxhighlight>
  
 
* From pointer-to-(possibly-qualified-)void-or-object type&nbsp;<code>P</code> to signed integer type&nbsp;<code>S</code>:
 
* From pointer-to-(possibly-qualified-)void-or-object type&nbsp;<code>P</code> to signed integer type&nbsp;<code>S</code>:
<code>[c]
+
<syntaxhighlight lang="cpp">
 
P p;
 
P p;
 
S s = SAL_INT_CAST(S, (sal_IntPtr) p);
 
S s = SAL_INT_CAST(S, (sal_IntPtr) p);
</code>
+
</syntaxhighlight>
  
 
* From signed integer type&nbsp;<code>S</code> to pointer-to-(possibly-qualified-)void-or-object type&nbsp;<code>P</code>:
 
* From signed integer type&nbsp;<code>S</code> to pointer-to-(possibly-qualified-)void-or-object type&nbsp;<code>P</code>:
<code>[c]
+
<syntaxhighlight lang="cpp">
 
S s;
 
S s;
 
P p = (P) SAL_INT_CAST(sal_IntPtr, s);
 
P p = (P) SAL_INT_CAST(sal_IntPtr, s);
</code>
+
</syntaxhighlight>
  
 
===Pointer to function===
 
===Pointer to function===
  
 
A pointer to function is not compatible with a pointer to object, so code like
 
A pointer to function is not compatible with a pointer to object, so code like
<code>[cpp]
+
<syntaxhighlight lang="cpp">
 
void * osl::Module::getSymbol(rtl::OUString const &);
 
void * osl::Module::getSymbol(rtl::OUString const &);
 
typedef void (* MyFunc)();
 
typedef void (* MyFunc)();
 
MyFunc f = module.getSymbol("myFunc");
 
MyFunc f = module.getSymbol("myFunc");
</code>
+
</syntaxhighlight>
 
will not compile.  The only places were converting between pointer to function and pointer to object (typically <code>void *</code>) should be necessary are <code>osl::Module::getSymbol</code> and <code>osl::Module::getUrlFromAddress</code>, and in both cases there are additional functions (<code>osl::Module::getFunctionSymbol</code> and an overload with <code>oslGenericFunction</code> of <code>osl::Module::getUrlFromAddress</code>) that handle the problem internally.  Use them!
 
will not compile.  The only places were converting between pointer to function and pointer to object (typically <code>void *</code>) should be necessary are <code>osl::Module::getSymbol</code> and <code>osl::Module::getUrlFromAddress</code>, and in both cases there are additional functions (<code>osl::Module::getFunctionSymbol</code> and an overload with <code>oslGenericFunction</code> of <code>osl::Module::getUrlFromAddress</code>) that handle the problem internally.  Use them!
  
Line 95: Line 147:
  
 
In C++, use a <code>using</code> declaration to make visible (virtual) function declarations from a base clase that would be hidden by a function declaration in a derived class:
 
In C++, use a <code>using</code> declaration to make visible (virtual) function declarations from a base clase that would be hidden by a function declaration in a derived class:
<code>[cpp]
+
<syntaxhighlight lang="cpp">
 
class Base {
 
class Base {
 
public:
 
public:
Line 106: Line 158:
 
   virtual void f(int);
 
   virtual void f(int);
 
};
 
};
</code>
+
</syntaxhighlight>
  
 
Make sure to apply the correct access control to the <code>using</code> declaration:
 
Make sure to apply the correct access control to the <code>using</code> declaration:
<code>[cpp]
+
<syntaxhighlight lang="cpp">
 
class Base {
 
class Base {
 
protected:
 
protected:
Line 120: Line 172:
 
   using Base::f;
 
   using Base::f;
 
};
 
};
</code>
+
</syntaxhighlight>
  
If you can, avoid such constructs as in the above example, by chosing different names for the two functions <code>f</code>. If on the other hand the function f is clearly meant to be overloaded, then make all instances of <code>f</code> virtual in the base class.
+
If you can, avoid such constructs as in the above example, by choosing different names for the two functions <code>f</code>. If, on the other hand, the function f is clearly meant to be overloaded, then make all instances of <code>f</code> virtual in the base class.
  
 
===Unused parameters===
 
===Unused parameters===
Line 128: Line 180:
 
There are three cases where a parameter of a function is not actually used in the definition of the function:
 
There are three cases where a parameter of a function is not actually used in the definition of the function:
  
* The function has to have a certain signature (it is a virtual function, shall be used with a certain function pointer type, or is part of a stable API that cannot be changed).  In those cases, leave out the paramter name in the function definition (C++) or use
+
* The function has to have a certain signature (it is a virtual function, shall be used with a certain function pointer type, or is part of a stable API that cannot be changed).  In those cases, leave out the parameter name in the function definition (C++) or use
<code>[c]
+
<syntaxhighlight lang="cpp">
 
(void) paramName; /* avoid warning about unused parameter */
 
(void) paramName; /* avoid warning about unused parameter */
</code>
+
</syntaxhighlight>
 
at the start of the function body (C).
 
at the start of the function body (C).
  
* The function uses the paramter only in an <code>#ifdef</code>-block, or only in <code>OSL_ASSERT</code> etc. debug code.  If the first case occurs in C++ code, fix it by using the same <code>#ifdef</code> around the parameter name in the function header.  In all other cases, fix it by using
+
* The function is created by the Macro IMPL_LINK( Class, Method, ArgType, ArgName ) from tools/inc/link.hxx or similar. Use EMPTYARG as Parametername as in
<code>[c]
+
<syntaxhighlight lang="cpp">
 +
IMPL_LINK( Class, Method, ArgType, EMPTYARG )
 +
</syntaxhighlight>
 +
EMPTYARG is defined empty, so this will result in leaving out the parameter name as suggested above.
 +
 
 +
* The function uses the parameter only in an <code>#ifdef</code>-block, or only in <code>OSL_ASSERT</code> etc. debug code.  If the first case occurs in C++ code, fix it by using the same <code>#ifdef</code> around the parameter name in the function header.  In all other cases, fix it by using
 +
<syntaxhighlight lang="cpp">
 
(void) paramName; /* avoid warning about unused parameter */
 
(void) paramName; /* avoid warning about unused parameter */
</code>
+
</syntaxhighlight>
 
at the start of the function body.
 
at the start of the function body.
  
Line 149: Line 207:
  
 
* Header files included directly from the system, or header files delivered from external code included in the OOo code base but which we cannot reasonably make <code>testhxx</code>-clean:  Surround that code (e.g., the <code>#include</code>-directives for those headers) by
 
* Header files included directly from the system, or header files delivered from external code included in the OOo code base but which we cannot reasonably make <code>testhxx</code>-clean:  Surround that code (e.g., the <code>#include</code>-directives for those headers) by
<code>[c]
+
<syntaxhighlight lang="cpp">
 
#if defined __GNUC__
 
#if defined __GNUC__
 
#pragma GCC system_header
 
#pragma GCC system_header
Line 157: Line 215:
 
#pragma warning(push, 1)
 
#pragma warning(push, 1)
 
#endif
 
#endif
</code>
+
</syntaxhighlight>
 
and
 
and
<code>[c]
+
<syntaxhighlight lang="cpp">
 
#if defined __SUNPRO_CC
 
#if defined __SUNPRO_CC
 
#pragma enable_warn
 
#pragma enable_warn
Line 165: Line 223:
 
#pragma warning(pop)
 
#pragma warning(pop)
 
#endif
 
#endif
</code>
+
</syntaxhighlight>
(probably together with an explanatory comment).  <em>Due to the nature of the GCC solution, this does not work directly within the compilation unit's main (<code>.c</code>, <code>.cxx</code>) file, only within a (<code>.h</code>, <code>.hxx</code>) file included directly or indirectly.</em>
+
(probably together with an explanatory comment).  <em>Due to the nature of the GCC solution, this does not work directly within the compilation unit's main (<code>.c</code>, <code>.cxx</code>) file, only within a (<code>.h</code>, <code>.hxx</code>) file included directly or indirectly.</em> Even more important, because the <code>system_header</code> pragma affects the ''rest of the current header'', the entire warnings guard mechanism should be separated into a ''header file on its own'', including just the file in question. From the original file then include that wrapper file.
  
 
* Code generated by <code>flex</code> or <code>bison</code>:  The solution here is similar to the solution in the previous item&mdash;disable all warnings in the generated code.  However, this has two implications:  For one, it means that also all warnings from our client code integrated into the <code>flex</code>/<code>bison</code> output are suppressed; this is not really that large an issue, given the relatively small number of <code>flex</code>/<code>bison</code> files we use.  For another, due to the nature of the GCC solution, it means that you need to introduce a wrapper file.  The individual steps for a given <code>flex</code>/<code>bison</code> file named <code>example.ly</code>, translated to <code>$(MISC)$/example.cxx</code>, are as follows:  First, change <code>example.ly</code> by adding
 
* Code generated by <code>flex</code> or <code>bison</code>:  The solution here is similar to the solution in the previous item&mdash;disable all warnings in the generated code.  However, this has two implications:  For one, it means that also all warnings from our client code integrated into the <code>flex</code>/<code>bison</code> output are suppressed; this is not really that large an issue, given the relatively small number of <code>flex</code>/<code>bison</code> files we use.  For another, due to the nature of the GCC solution, it means that you need to introduce a wrapper file.  The individual steps for a given <code>flex</code>/<code>bison</code> file named <code>example.ly</code>, translated to <code>$(MISC)$/example.cxx</code>, are as follows:  First, change <code>example.ly</code> by adding
<code>[cpp]
+
<syntaxhighlight lang="cpp">
 
#if defined __GNUC__
 
#if defined __GNUC__
 
#pragma GCC system_header
 
#pragma GCC system_header
Line 177: Line 235:
 
#pragma warning(push, 1)
 
#pragma warning(push, 1)
 
#endif
 
#endif
</code>
+
</syntaxhighlight>
(probably together with an explanatory comment) near the top of the initial <code>%{...%}</code> section, after any <code>#include</code>s (which might contain <code>#pragma disable_warn</code>/<code>#pragma enable_warn</code> pairs for <code>__SUNPRO_CC</code> and thus would enable warnings again in the following code).  Second, create a new file named <code>wrap_example.cxx</code> in the same source directory as <code>exmaple.ly</code>, containing
+
(probably together with an explanatory comment) at the very end of the initial <code>%{...%}</code> section (for <code>_MSC_VER</code>, <code>#pragma warning(push, 1)</code> might not disable all the warnings that occur; in that case, add <code>#pragma warning(disable: NNN1 NNN2 ...)</code> after it to disable specific warnings <code>NNN1</code>, <code>NNN2</code>, etc.).  Second, create a new file named <code>wrap_example.cxx</code> in the same source directory as <code>exmaple.ly</code>, containing
<code>[cpp]
+
<syntaxhighlight lang="cpp">
 
#include "example.cxx"
 
#include "example.cxx"
</code>
+
</syntaxhighlight>
 
Third, change the corresponding <code>makefile.mk</code> by adding
 
Third, change the corresponding <code>makefile.mk</code> by adding
 
<code>[]
 
<code>[]
 
INCPRE=$(MISC)
 
INCPRE=$(MISC)
 
</code>
 
</code>
to the top, by changing occurences of <code>example.obj</code> to <code>wrap_example.obj</code>, and by adding
+
to the top, by changing occurrences of <code>example.obj</code> to <code>wrap_example.obj</code>, and by adding
 
<code>[]
 
<code>[]
 
$(OBJ)$/wrap_example.obj: $(MISC)$/example.cxx
 
$(OBJ)$/wrap_example.obj: $(MISC)$/example.cxx
Line 195: Line 253:
 
</code>
 
</code>
 
at the bottom.
 
at the bottom.
 +
 +
 +
[[Category:Coding Standards]]

Latest revision as of 12:02, 21 June 2021

We are currently on the way towards a completely warning-free OOo C/C++ code base. And once we have reached that, we want to keep the code clean!

We reached a first milestone by integrating CWS warnings01 into SRC680m173. Since then, and at least on the unxlngi6, unxsoli4, unxsols4, and wntmsci10 platforms, the C/C++ compilers are used with rather aggressive warning levels, and in most CVS modules warnings are treated as errors that break the build. Some CVS modules have not yet been made warning-free.

What changed?

Since SRC680m173, any new or modified C/C++ code that produces warnings will break the build. (Unless, of course, you fail to pass --enable-werror to configure.) Ideally, to be sure that integrating a CWS will not break the build, the CWS should now be built on every warning-free platform (i.e., at least unxlngi6, unxsoli4, unxsols4, and wntmsci10), with all supported compiler versions, in both product and non-product variants, and with and without the debug=x build switch before nominating it for integration. Practically, a reasonable subset of these will have to suffice.

For unxlngi6, we concentrated on GCC 3.4.1, GCC 4.0.2/4.0.3, and GCC 4.1.1, while other GCC versions probably produce slightly different warnings (that could break the build now on those GCC versions).

Porters are encouraged to make additional platforms warning-free. Since most ports are using GCC, there should not be too many surprises. Any necessary changes to the platform-specific .mk file in solenv/inc can be modelled after solenv/inc/unxlngi6.mk. In short, you need a bunch of CFLAGSxxx (see interface-announce C/C++ compiler warnings) and MODULES_WITH_WARNINGS or COMPILER_WARN_ERRORS (see interface-announce MODULES_WITH_WARNINGS).

The wall=x build switch no longer has any effect on the warning-free platforms. Once all platforms are warning-free, we can remove it completely.

The missing modules

If your job is to clean up a given CVS module, here are some practical guidelines:

  • Remove the module from the MODULES_WITH_WARNINGS list in the platform-specific .mk files in solenv/inc.
  • Make sure your module builds and delivers successfully with build && deliver (and all the variants mentioned above).
  • If your module contains external code which we do not want to clean up, set EXTERNAL_WARNINGS_NOT_ERRORS := TRUE at the beginning of the corresponding makefile.mk, so that warnings do not lead to errors there.
  • In any case, for any header file delivered from your module, make sure that testhxx on the delivered header file is successful. (Note that testhxx only works for headers delivered to the solver, not for local headers within a CVS module, as it calls the compiler with only a fixed set of include directories.)
  • If you encounter a warning that cannot reasonably be worked around, discuss that problem on dev@openoffice.org. One outcome of that discussion can be that we globally disable that particular warning. To see which warnings are already globally disabled on a given platform, look at the settings for CFLAGSWARNCC, CFLAGSWARNCXX, CFLAGSWALLCC, and/or CFLAGSWALLCXX in the platform-specific .mk file in solenv/inc (unxlngi6.mk, unxsoli4.mk, unxsols4.mk, and wntmsci10.mk).

See the current warning-free code status.

Tips for Warning-free code

The following is a list of issues you might come across when making existing code warning-free, or when writing new, warning-free code:

Integral conversions

Sometimes, a cast between integral types is needed to avoid a warning. In such a case, use sal::static_int_cast (C++) or SAL_INT_CAST (C) to make that cast stick out—it will need to be adapted when the type of any of the involved expressions changes later on:

sal_Int32 n;
std::vector<char> v;
if (n >= 0 && sal::static_int_cast<sal_uInt32>(n) == v.size()) ...

Note that you might get the warning C4244 (possible loss of data) on Windows also for the following situation:

short a = 0, b = 0;
a += b;  // warning C4244 on Windows

This is because a and b are converted to int, then added and the result is then again cast back to short. This is especially nasty, as it also happens when you use xub_StrLen, but only if it is a typedf to USHORT and not to sal_uInt32, i.e. only as long as STRING32 is not defined in solar.h. The warning does not occur when you use operator+ instead of operator+=.

short a = 0, b = 0;
a = a + b;

Invalid enum case values

Code like

enum E { A=1, B=2, C=4 } e;
switch (e) {
case A: ...
case B|C: ...
}

will cause wntmsci11 to complain with warning C4063: case '6' is not a valid value for switch of enum 'E'. The cause of the warning is that E is not meant be an enumerated type, but a bit field type. enum is commonly used in C and C++ to initialize constants; in this case, constants for individual bits.

A better way to write this would be to typedef E to be an unsigned int (a better type for bit fields); and then define the constants for the individual bits using an anonymous enum. (A downside is that the documentation for the type E will not contain the constants the user needs to use.)

typedef unsigned int E;
E e;
enum { A=1, B=2, C=4 };
switch (e) {
case A: ...
case B|C: ...
}

If you do not have access to the definition of the type E, it can still be worked around by promoting e to its underlying integral type:

...
switch (+e) {
...

Casting between pointer and integer types

C++

  • From pointer-to-(possibly-cv-qualified-)void-or-object type P to unsigned integral type U:
P p;
U u = sal::static_int_cast<U>(reinterpret_cast<sal_uIntPtr>(p));
  • From unsigned integral type U to pointer-to-(possibly-cv-qualified-)void-or-object type P:
U u;
P p = reinterpret_cast<P>(sal::static_int_cast<sal_uIntPtr>(u));
  • From pointer-to-(possibly-cv-qualified-)void-or-object type P to signed integral type S:
P p;
S s = sal::static_int_cast<S>(reinterpret_cast<sal_IntPtr>(p));
  • From signed integral type S to pointer-to-(possibly-cv-qualified-)void-or-object type P:
S s;
P p = reinterpret_cast<P>(sal::static_int_cast<sal_IntPtr>(s));

C

  • From pointer-to-(possibly-qualified-)void-or-object type P to unsigned integer type U:
P p;
U u = SAL_INT_CAST(U, (sal_uIntPtr) p);
  • From unsigned integer type U to pointer-to-(possibly-qualified-)void-or-object type P:
U u;
P p = (P) SAL_INT_CAST(sal_uIntPtr, u);
  • From pointer-to-(possibly-qualified-)void-or-object type P to signed integer type S:
P p;
S s = SAL_INT_CAST(S, (sal_IntPtr) p);
  • From signed integer type S to pointer-to-(possibly-qualified-)void-or-object type P:
S s;
P p = (P) SAL_INT_CAST(sal_IntPtr, s);

Pointer to function

A pointer to function is not compatible with a pointer to object, so code like

void * osl::Module::getSymbol(rtl::OUString const &);
typedef void (* MyFunc)();
MyFunc f = module.getSymbol("myFunc");

will not compile. The only places were converting between pointer to function and pointer to object (typically void *) should be necessary are osl::Module::getSymbol and osl::Module::getUrlFromAddress, and in both cases there are additional functions (osl::Module::getFunctionSymbol and an overload with oslGenericFunction of osl::Module::getUrlFromAddress) that handle the problem internally. Use them!

Name hiding

In C++, use a using declaration to make visible (virtual) function declarations from a base clase that would be hidden by a function declaration in a derived class:

class Base {
public:
  virtual void f(int);
  virtual void f(double);
};
class Derived: public Base {
public:
  using Base::f;
  virtual void f(int);
};

Make sure to apply the correct access control to the using declaration:

class Base {
protected:
  void f();
};
class Derived: public Base {
public:
  void f(int);
protected:
  using Base::f;
};

If you can, avoid such constructs as in the above example, by choosing different names for the two functions f. If, on the other hand, the function f is clearly meant to be overloaded, then make all instances of f virtual in the base class.

Unused parameters

There are three cases where a parameter of a function is not actually used in the definition of the function:

  • The function has to have a certain signature (it is a virtual function, shall be used with a certain function pointer type, or is part of a stable API that cannot be changed). In those cases, leave out the parameter name in the function definition (C++) or use
(void) paramName; /* avoid warning about unused parameter */

at the start of the function body (C).

  • The function is created by the Macro IMPL_LINK( Class, Method, ArgType, ArgName ) from tools/inc/link.hxx or similar. Use EMPTYARG as Parametername as in
IMPL_LINK( Class, Method, ArgType, EMPTYARG )

EMPTYARG is defined empty, so this will result in leaving out the parameter name as suggested above.

  • The function uses the parameter only in an #ifdef-block, or only in OSL_ASSERT etc. debug code. If the first case occurs in C++ code, fix it by using the same #ifdef around the parameter name in the function header. In all other cases, fix it by using
(void) paramName; /* avoid warning about unused parameter */

at the start of the function body.

  • The function once used the parameter, but does no longer do so. In that case, clean up the function declaration and all uses of the function.

When all else fails

There are cases where the only sensible solution is to suppress warnings for a given chunk of code:

  • As explained above, in external code which we do not want to clean up, use EXTERNAL_WARNINGS_NOT_ERRORS := TRUE so that warnings do not lead to errors there.
  • Header files included directly from the system, or header files delivered from external code included in the OOo code base but which we cannot reasonably make testhxx-clean: Surround that code (e.g., the #include-directives for those headers) by
#if defined __GNUC__
#pragma GCC system_header
#elif defined __SUNPRO_CC
#pragma disable_warn
#elif defined _MSC_VER
#pragma warning(push, 1)
#endif

and

#if defined __SUNPRO_CC
#pragma enable_warn
#elif defined _MSC_VER
#pragma warning(pop)
#endif

(probably together with an explanatory comment). Due to the nature of the GCC solution, this does not work directly within the compilation unit's main (.c, .cxx) file, only within a (.h, .hxx) file included directly or indirectly. Even more important, because the system_header pragma affects the rest of the current header, the entire warnings guard mechanism should be separated into a header file on its own, including just the file in question. From the original file then include that wrapper file.

  • Code generated by flex or bison: The solution here is similar to the solution in the previous item—disable all warnings in the generated code. However, this has two implications: For one, it means that also all warnings from our client code integrated into the flex/bison output are suppressed; this is not really that large an issue, given the relatively small number of flex/bison files we use. For another, due to the nature of the GCC solution, it means that you need to introduce a wrapper file. The individual steps for a given flex/bison file named example.ly, translated to $(MISC)$/example.cxx, are as follows: First, change example.ly by adding
#if defined __GNUC__
#pragma GCC system_header
#elif defined __SUNPRO_CC
#pragma disable_warn
#elif defined _MSC_VER
#pragma warning(push, 1)
#endif

(probably together with an explanatory comment) at the very end of the initial %{...%} section (for _MSC_VER, #pragma warning(push, 1) might not disable all the warnings that occur; in that case, add #pragma warning(disable: NNN1 NNN2 ...) after it to disable specific warnings NNN1, NNN2, etc.). Second, create a new file named wrap_example.cxx in the same source directory as exmaple.ly, containing

#include "example.cxx"

Third, change the corresponding makefile.mk by adding [] INCPRE=$(MISC) to the top, by changing occurrences of example.obj to wrap_example.obj, and by adding [] $(OBJ)$/wrap_example.obj: $(MISC)$/example.cxx and/or [] $(SLO)$/wrap_example.obj: $(MISC)$/example.cxx at the bottom.

Personal tools