Difference between revisions of "Writing warning-free code"

From Apache OpenOffice Wiki
Jump to: navigation, search
(testhxx clarification)
Line 9: Line 9:
 
* 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.
 
* 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.
 
* 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 the <code>"$(COMEX)"==10</code> branch of <code>wnt.mk</code>).
 
* Commit your changes (do not forget to add your module to the CWS before making any changes).  However, remember that this CWS will probably last for a rather long time, and will be resync'ed quite often, so take care that the changes you commit do not lead to any unnecessary resync conflicts (e.g., do not commit any changes that only change whitespace).<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!
 
* Commit your changes (do not forget to add your module to the CWS before making any changes).  However, remember that this CWS will probably last for a rather long time, and will be resync'ed quite often, so take care that the changes you commit do not lead to any unnecessary resync conflicts (e.g., do not commit any changes that only change whitespace).<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!

Revision as of 16:39, 1 March 2006

On CWS 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!

On that CWS, you currently need to build with the build switch werror=1 to turn compiler warnings into errors, as for example in

build werror=1

There is also a build switch wall=1 to switch on as many compiler warnings as usefull, but its effect is already switched on per default on the four Hamburg-relevant platforms unxlngi6, unxsoli4, unxsols4, and wntmsci10. 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:

  • Work directly on the CWS (/cws/cws03/warnings01 in Hamburg).
  • Make sure your module builds and delivers successfully with build werror=1 && deliver.
    Note that you can also set an environment variable "werror" to "1" (or "true" or whatever) - then a simply build will do.
  • 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 the "$(COMEX)"==10 branch of wnt.mk).
  • Commit your changes (do not forget to add your module to the CWS before making any changes). However, remember that this CWS will probably last for a rather long time, and will be resync'ed quite often, so take care that the changes you commit do not lead to any unnecessary resync conflicts (e.g., do not commit any changes that only change whitespace).
    Attention: If you do changes to your module before doing a cwsadd, then cwsadd will throw away all your changes!
  • In a first step, we concentrate on cleaning up the code for unxlngi6.pro. In a second step, the other Hamburg platforms (unxlngi6, unxsoli4.pro, unxsols4, unxsols4.pro, wntmsci10, and wntmsci10.pro) will follow.
  • Anounce in a mail to all people involved in the CWS that your module is cleaned up for unxlngi6.pro (or even more platforms).
  • warningfreecode.csv in Hamburg represents the current status. That document is updated by Stephan Bergmann from the anouncement mails; do not modify it yourself.

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: [cpp] sal_Int32 n; std::vector<char> v; if (n >= 0 && sal::static_int_cast<sal_uInt32>(n) == v.size()) ...

Casting between pointer and integer types

C++

  • From pointer-to-(possibly-cv-qualified-)void-or-object type P to unsigned integral type U:

[cpp] P p; U u = sal::static_int_cast(reinterpret_cast<sal_uIntPtr>(p)); </code>

  • From unsigned integral type <code>U</code> to pointer-to-(possibly-cv-qualified-)void-or-object type <code>P</code>:

<code>[cpp] U u;

P p = reinterpret_cast

(sal::static_int_cast<sal_uIntPtr>(u)); </code>

  • From pointer-to-(possibly-cv-qualified-)void-or-object type <code>P</code> to signed integral type <code>S</code>:
<code>[cpp] P p; S s = sal::static_int_cast(reinterpret_cast<sal_IntPtr>(p)); </code>
  • From signed integral type <code>S</code> to pointer-to-(possibly-cv-qualified-)void-or-object type <code>P</code>:
<code>[cpp] S s; P p = reinterpret_cast<P>(sal::static_int_cast<sal_IntPtr>(s)); </code>

C

  • From pointer-to-(possibly-qualified-)void-or-object type <code>P</code> to unsigned integer type <code>U</code>:

<code>[c] P p; U u = SAL_INT_CAST(U, (sal_uIntPtr) p); </code>

  • From unsigned integer type <code>U</code> to pointer-to-(possibly-qualified-)void-or-object type <code>P</code>:

<code>[c] U u; P p = (P) SAL_INT_CAST(sal_uIntPtr, u); </code>

  • From pointer-to-(possibly-qualified-)void-or-object type <code>P</code> to signed integer type <code>S</code>:

<code>[c] P p; S s = SAL_INT_CAST(S, (sal_IntPtr) p); </code>

  • From signed integer type <code>S</code> to pointer-to-(possibly-qualified-)void-or-object type <code>P</code>:

<code>[c] S s; P p = (P) SAL_INT_CAST(sal_IntPtr, s); </code>

Pointer to function

A pointer to function is not compatible with a pointer to object, so code like <code>[cpp] void * osl::Module::getSymbol(rtl::OUString const &); typedef void (* MyFunc)(); MyFunc f = module.getSymbol("myFunc"); </code> 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!

Name hiding

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] class Base { public:

 virtual void f(int);
 virtual void f(double);

}; class Derived: public Base { public:

 using Base::f;
 virtual void f(int);

}; </code>

Make sure to apply the correct access control to the <code>using</code> declaration: <code>[cpp] class Base { protected:

 void f();

}; class Derived: public Base { public:

 void f(int);

protected:

 using Base::f;

}; </code>

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.

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 paramter name in the function definition (C++) or use

<code>[c] (void) paramName; /* avoid warning about unused parameter */ </code> 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

<code>[c] IMPL_LINK( Class, Method, ArgType, EMPTYARG ) </code> EMPTYARG is defined empty, so this will result in leaving out the parameter name as suggested above.

  • 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

<code>[c] (void) paramName; /* avoid warning about unused parameter */ </code> 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 <code>EXTERNAL_WARNINGS_NOT_ERRORS := TRUE</code> 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 <code>testhxx</code>-clean: Surround that code (e.g., the <code>#include</code>-directives for those headers) by

<code>[c]

  1. if defined __GNUC__
  2. pragma GCC system_header
  3. elif defined __SUNPRO_CC
  4. pragma disable_warn
  5. elif defined _MSC_VER
  6. pragma warning(push, 1)
  7. endif

</code> and <code>[c]

  1. if defined __SUNPRO_CC
  2. pragma enable_warn
  3. elif defined _MSC_VER
  4. pragma warning(pop)
  5. endif

</code> (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 (<code>.c</code>, <code>.cxx</code>) file, only within a (<code>.h</code>, <code>.hxx</code>) file included directly or indirectly.

  • Code generated by <code>flex</code> or <code>bison</code>: 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 <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]

  1. if defined __GNUC__
  2. pragma GCC system_header
  3. elif defined __SUNPRO_CC
  4. pragma disable_warn
  5. elif defined _MSC_VER
  6. pragma warning(push, 1)
  7. endif

</code> (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]

  1. include "example.cxx"

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

Personal tools