Created attachment 48254 [details]
patch with some fixes. Needs review

wae4extensions cws has been created + I have added Michael Sicotte on CC

All warnings have been seen on Mac OS X / Aqua, and some need confirmation. 

I'll attach the initial log file.

thanks in advance for any review or help

Created attachment 48255 [details]
initial log (all warnings inside)

Thansk to Pavel Janik and Michael Sicotte for some precious advices :)

The comments to the last patch:

extensions/source/abpilot/abspilot.cxx: use default: instead of
listing them all

extensions/source/abpilot/fieldmappingimpl.cxx: looks like it is an
adept to while cycle instead ;-)

extensions/source/bibliography/bibconfig.hxx: why do you change
header?

extensions/source/bibliography/bibcont.hxx: why do you change header?

extensions/source/bibliography/bibview.cxx:

Changes like this:

-		e;	// make compiler happy
-		DBG_ERROR("::createGridModel: something went wrong !");
+		(void) e;	// make compiler happy
+        DBG_ERROR("::createGridModel: something went wrong !");

change indentation of the second line - why?

I wonder who wrote original here, because there are 10+ times the same
code lines.

extensions/source/bibliography/loadlisteneradapter.hxx: change is OK,
but please take back indentation changes and the line deleted.

extensions/source/plugin/base/context.cxx: why do you change the class
declaration? Please fix the bad order inits...

extensions/source/propctrlr/genericpropertyhandler.cxx: changing
header/ order of private members: why?

extensions/source/scanner/sane.cxx (and other places):
Do not use such casts in C++ code!
+	if( (unsigned int) pZero->size > sizeof( SANE_Word ) )

use default: instead of listing all cases.

This code:

@@ -442,6 +443,9 @@
                     break;
                 case SANE_TYPE_FIXED:
                 case SANE_TYPE_INT:
+                case SANE_TYPE_BOOL:
+                case SANE_TYPE_STRING:
+                case SANE_TYPE_GROUP:

Is actually changing the meaning!

extensions/source/update/check/updatecheck.cxx:
-            ShutdownThread *pShutdownThread = new ShutdownThread( m_xContext );

This actually can also change the behaviour if ShutDownThread is
creating the thread or something.

I'd volunteer to do the warning-freeness for propctrlr, dbpilots, abpilot,
logging (those directories in extensions/source are mine) for unxlngi6 and
wntmsci10.

Created attachment 48502 [details]
make fs' sub modules in extensions/source warning-free on wntmsci10/.pro

With the attached patch, abpilot, dbpilots, propctrlr compile without warning on
wntmsci10/.pro. logging (also mine) already compiles without warnings. resource
(halfway mine) also compiles without warning.

Created attachment 48507 [details]
make fs' sub modules in extensions/source warning-free on wntmsci10/.pro and unxlngi6/.pro

second patch makes mentioned modules warning-free on unxlngi6/.pro, too

allow me to comment the WaE_extensions4.diff patch (some of this has been sad by
Pavel already, but allow me to bee too lazy to fiddle out):

- The change to MacPluginComm::~MacPluginComm looks bad, it removes essential
  code (the call to waitpid, for instance), and will most probably not compile in a
  non-product version (--enable-dbgutil) or when compiled with debug (dmake debug=1)

-The change in plugin/base/plmodel.cxx (2 occurances) should probably better be
    if( rValue.getValueTypeClass() == TypeClass_STRING )
  getValueTypeClass returns a css.uno.TypeClass enum value, so let's simply
  compare it with such a value, instead of casting

- commenting out the guards in plugin/base/xplugin.cxx is definitely a bad idea,
  they're in for thread safety reasons. What was the problem with the original
lines?

- scanner/sane.cxx
  I'm not sure about changing
    if( pZero->size > sizeof( SANE_Word ) )
  to
    if( (unsigned int) pZero->size > sizeof( SANE_Word ) )
  Shouldn't the "unsigned int" better be a "size_t"? There are multiple occurences
  of this pattern, where I think the return value of sizeof-expressions should be
  compared with size_t instead of unsigned int.

- the two new cases in sane.cxx:863 might be worth an assertion. Not sure,
  don't know the code.

- sane.cxx:987 perhaps should use "sal::static_cast< SANE_Unit, size_t >" (see
  sal/types.h), or simply cast nUnit to size_t.

- The change in saldlg.cxx:442 seems to change the behavior, in that the three
  additional cases now do something different than before.
  A dedicated case-branch which does nothing (but raise an
  OSL_ENSURE( false, "..." )) might be better here.

- removing "SvXMLElementExport aElemG" in svgaction.cxx:737 is a bad idea,
  definitely. SvXMLElementExport is a class with non-trivial code in its ctor/dtor.
  I suppose the compiler complained about aElemG in line 767 hiding the aElemG
  in line 740. In this case, simply rename the second aElemG.

- removing the "new ShutdownThread" in updatecheck.cxx most probably
  also has unwanted side effects. You should clarify with the code owner,
  or add an "(void)pShutdownThread" after the declaration.

@pjanik:
> I wonder who wrote original here, because there are 10+ times the same
> code lines.

Somebody who is not here anymore to blame him :)

Created attachment 48511 [details]
be warning-free in abpilot, bibliography, dbpilots, logging, propctrlr, resource; on wntmsci10/.pro and unxlngi6/.pro

the new patch adds warning-freeness to the bibliography sub module, based on
Eric's initial patch (which had some problems with undesired side effects).

Created attachment 48519 [details]
the same sub modules, but now also warning free on unxsoli4.pro

The last patch makes the aforementioned sub directories warning free on
unxsoli4.pro, unxlngi6, unxlngi6.pro, wntmsci10, wntmsci10.pro.

Note that this now includes a patch to comphelper, since unxsoli4 complained
about a helper class therein which seems is nowhere else used.

Eric, what about promoting the CWS from "planned" to "new"?

Done: cws wae4extensions is in new state, task i81612 has been added, and extensions belong to the cws

Sorry for not being working on that cws currently: I'm at Apple Expo Paris until the end of the week, but I 
promise to continue next week

Thanks. Am I allowed to add comphelper (my code needs it, see above) and solenv
(for removing the wae-exception for module extensions) to the CWS?

Committed my changes to extensions.

Eric, any objections if I continue with some more sub modules on my platforms?

Created attachment 48594 [details]
make extensions\source\ole warning-free on wntmsci10/.pro

fs->jl:
The attached patch (wae_extensions_source_ole) is my attempt to make, well,
extensions\source\ole warning-free. I'm unsure about some of those, in
particular, the necessary change to oleobjw.cxx might suggest there's a bug in
the code (I did not find documentation about GetRefTypeOfImplType accepting a -1
for some purpose, but didn't try very hard).

Do you mind reviewing the changes?

I'll have a look.

Created attachment 48626 [details]
updated patch for extensions/source/ole, using more sal::static_int_casts per JL's request

So, I could not resist ....

Talked with some people owning some code with remaining warnings, fixed some
obvious warnings myself (using your patch, Eric, especially for the
member-re-ordering required by GCC's warning).

extensions is now warning-free on wntmsci10/.pro, unxlngi6/.pro, unxsoli4.pro,
unxsols4.pro.

Which let's me assume it's also warning-free on Mac - sorry if I this was a kind
of (un/friendly) takeover of this issue :)

Eric, what about adding comphelper and solenv to the CWS?

*** Issue 69282 has been marked as a duplicate of this issue. ***

@fs/ericb:  Can we get this into OOo 2.4 (see
<http://wiki.services.openoffice.org/wiki/Warning-free_code_status>)?

@sb : yes, 2.4 seems to be possible.

@fs: In fact, I'd prefer not add new module in the cws:  extensions is in the
cws name, and could lead to confusion if something wrong occurs, or somebody
searching for changes in comphelper and/or solenv.

If we don't add comphelper, then extensions won't be warning-free: There's a
header delivered from comphelper which triggers a warning in
extensions/source/propctrlr, the fix for the warning must be done in comphelper.

If we don't add solenv, making the projects warning-free will not persist: There
is a mechanism in solenv which declares which modules are allowed to have
warnings as non-errors. See, for instance, solenv/inc/wntmsci10.mk (and the
respective files for all other platforms). Therein, extensions is declared as
"warnings are allowed".

Thus, we really need comphelper and solenv, or we won't have a real warning-free
extensions module.

ericb->fs

I think you are completely right, and add solenv and comphelper modules with comments in the cws is 
the right way to make extensions warning free here.

After some tries, EIS seems to have problems ?  Can you please try to add the modules this morning ? 
Else, I'll retry this afternoon, when I'll have more time

 
 

@all :  I'll resync wae4extensions with m231, so please stop commiting until I
confirm it's ok

Done:  wae4extensions is resync'ed with m231, solenv and comphelper have been added
I hope I did no mistake ( EIS is strange these days)

After a quick try, I want to thank you everybody who commited all the changes: this is really great to 
see less warnings !! 

FYI, I commited some changes in wae4extensions, because :

- I had a breaker in extensions/source/plugin/base/nfuncts.cxx
- some residuals warnings for aqua plugin

I'll attach the patch corresponding to my changes.

Can anybody review them ?  Thanks in advance :)

With current changes, I only get 2 last warnings:

/Users/ericb/Desktop/SRC680_m231/extensions/source/plugin/aqua/macmgr.cxx: In function 'bool 
CheckPlugin(const ByteString&, _STL::list<com::sun::star::plugin::PluginDescription*, 
_STL::allocator<com::sun::star::plugin::PluginDescription*> >&)':
/Users/ericb/Desktop/SRC680_m231/extensions/source/plugin/aqua/macmgr.cxx:100: warning: 
comparison between signed and unsigned integer expressions

/Users/ericb/Desktop/SRC680_m231/extensions/source/plugin/aqua/sysplug.cxx: In destructor 
'virtual MacPluginComm::~MacPluginComm()':
/Users/ericb/Desktop/SRC680_m231/extensions/source/plugin/aqua/sysplug.cxx:107: warning: 
unused variable 'nExit'

We are close :-) 

Created attachment 48675 [details]
last changes for wae4extensions cws

Note: I'm not really sure of the change I did in extensions/source/plugin/base/nfuncs.cxx

Your change to nfuncs.cxx was correct.

I introduced the NULLs you just removed while building unxlngi6/.pro, but I now
noticed I was wrong: The plugin/unx/sysplug.hxx file had a
  #include <npupp.h>
, which on my build system included a file which does *not* come from the OOo
source tree, and is newer than this. Thus, my compiler warned about missing
initializers.
The proper include is
  #include <npsdk/npupp.h>
, which also results in the warnings being gone.

I should probably send a mail to dev@ooo asking porters and distributions to
check this. I suppose there is some configure switch --with-system-npsdk (or
something like this), which will break now. In this case, we need to make
"<npupp.h>" vs. "<npsdk/npupp.h>" make depend on this configure-time setting.


All other changes look fine. I rebuilt extensions on wntmsci10/.pro,
unxlngi6/.pro, unxsols4/.pro, and unxsoli4.pro, all without any warnings. Great
stuff, that.

Eric, you should also remove the "extensions" module from the
MODULES_WITH_WARNINGS define in solenv/inc/unxmacx.mk. This ensures that
warnings are really treated as errors in future, so nobody can break what we
just reached.

... if you remember to config_office/configure --enable-werror

@sb:  thanks for the reminder 8) ( /me bad -> I completely forgot the configure flag ;) 

@fs:  with the two last commits I did, extensions is warning free on Mac OS X Aqua. Can you confirm my 
changes are ok ?  If they are, I'll do a complete build on Mac OS X Aqua to verify.

@all : who is volunteer for the code review and the cws QA ? 

I confirm, Aqua build is warning free on Mac OS X. The last test has to be done building X11 version.

... I meant Aqua is warning free building extensions (was already building comphelper)

The change to macmgr.cxx looks perfectly okay, the change to sysplug.cxx
(1.2.44.2) looks strange for me, but if your compiler says it's okay, then it's
okay I assume.

Out of interest: What was the compiler warning which let you make this change to
sysplug.cxx?

for the QA: I suggest you ask in dev@qa. I will also ask inside the Hamburg team
on monday, but dev@qa should also be involved, I'd say.

ericb@fs

Indeed, use two lines fixes the warning. This is strange, but the warning was : unused variable 'nExit'. 
I already met such cases in several places on Mac OS X (in old Carbon salframe implementation).


The log ( --enable-werror works ) :

ARTZ -DOJI -DSHAREDLIB -D_DLL_   -fno-exceptions -DEXCEPTIONS_OFF  -o ../../../unxmacxi.pro/
slo/sysplug.o /Users/ericb/Desktop/SRC680_m231/extensions/source/plugin/aqua/sysplug.cxx 
cc1plus: warnings being treated as errors
/Users/ericb/Desktop/SRC680_m231/extensions/source/plugin/aqua/sysplug.cxx: In destructor 
'virtual MacPluginComm::~MacPluginComm()':
/Users/ericb/Desktop/SRC680_m231/extensions/source/plugin/aqua/sysplug.cxx:108: warning: 
unused variable 'nExit'
dmake:  Error code 1, while making '../../../unxmacxi.pro/slo/sysplug.obj'
---* tg_merge.mk *---

ericb@fs 

This is not finished, but this issue is really great for me: I learned a lot with the fixes you, Pavel and 
Michael proposed ( I must admint I had no idea for some of them), and I'll keep the issue number as 
reminder in my bookmarks. This will certainly help me from time to time in the future ;-)

Last but not least, I'm glad to see the progress done since the beginning. Thank you very much !!

FYI: the last remaining modules on MacOSX are sw and autodoc. I never thought before, but I'll try to have 
a look a day.

Ah ....

A better fix then would be to keep the original line
  pid_t nExit = ...
and add an additional line
  (void)nExit;

In fact, nExit is used, but only when debug information is enabled ("dmake
debug=1"). See the #if below the line you changed, and
http://wiki.services.openoffice.org/wiki/Debug_Levels.

So, in a debug build, you would not have got a warning at all. The usual fix for
this kind of "unused" variables is the above
  (void)<varname>;
.

(
Of course, the *complete* solution for nitpickers would be
#if OSL_DEBUG_LEVEL > 1
  pid_t nExit =
#endif
    waitpid( ... );
#if OSL_DEBUG_LEVEL > 1
  fprintf( stderr, ... )
#endif
)

Other than that: you're welcome :)

regarding sw: In m231, the CWS swwarnings has been intergrated
(http://eis.services.openoffice.org/EIS2/cws.ShowCWS?Path=SRC680%2Fswwarnings).
Since the warnings you got on mac looked pretty similar to the ones on our
unxlngi6 platform, I suppose you also use some kind of GCC. Which means that
most of the warnings, if not all, might already be gone in m231.

ericb@fs :

Maybe I'm wrong, but what about use:

Index: extensions/source/plugin/aqua/sysplug.cxx
===============================================================
====
RCS file: /cvs/util/extensions/source/plugin/aqua/sysplug.cxx,v
retrieving revision 1.2.44.2
diff -u -r1.2.44.2 sysplug.cxx
--- extensions/source/plugin/aqua/sysplug.cxx   4 Oct 2007 15:19:14 -0000       1.2.44.2
+++ extensions/source/plugin/aqua/sysplug.cxx   4 Oct 2007 20:41:10 -0000
@@ -104,10 +104,11 @@
        if( m_nCommPID != -1 && m_nCommPID != 0 )
     {
         int status = 16777216;
-        pid_t nExit; 
-       nExit = waitpid( m_nCommPID, &status, WUNTRACED );
 #if OSL_DEBUG_LEVEL > 1
-        fprintf( stderr, "child %d (plugin app child %d) exited with status %d\n", nExit, m_nCommPID, 
WEXITSTATUS(status) );
+       pid_t nExit = waitpid( m_nCommPID, &status, WUNTRACED );
+       fprintf( stderr, "child %d (plugin app child %d) exited with status %d\n", nExit, m_nCommPID, 
WEXITSTATUS(status) );
+#else
+       waitpid( m_nCommPID, &status, WUNTRACED );
 #endif
     }
 }

That works, too, but has the disadvantage of duplicating the waitpid code.
Duplicated code will nearly always get out of sync, it's just a matter of time,
so I always recommend not duplicating anything, were possible.

@fs

Sorry, I'm not sure to get/understand what is duplicated ? You mean in runtime ? Or just write waitpid() 
.. someting ?

If what i commited is wrong, I'll simply reverse the change, and use the one you adviced. 

Other point:  Christian Lohmaier found a breaker (because of a warning) in 
extensions/source/plugin/unx/npnapi.cxx

I commited the change we discussed with Pavel, and I'm waiting for Christian confirmation
that the breaker is fixed.

After all the points above are fixed, I think I can set the cws as Ready for QA. What do you think ?

@ericb:

With the above patch, you have two times the code
  waitpid( m_nCommPID, &status, WUNTRACED )
, once for debug builds, and once for non-debug builds. If somebody decides to,
say, replace &status with something else, then chances are good that one of the
two incarnations is overlooked - this is what I mean with getting out of sync.

Such a problem is rather theoretic in this particular case, since both
incarnations are close to each other. It's just a matter of principle. I learned
that duplicated code leads to fixing bugs (or changing facets) in one
incarnation only, and not in all duplicates ...

That said, all suggested patches are fine. It's just a matter of taste.

@sb

cloph found two other breakers in extensions/source/plugin/unx

They are now fixed, and wae4extensions is warning free  for Mac OS X Aqua and X11 too  :-)

Last task: find a volunteer to QA the cws 

oops ..  s/@sb/@fs/  :-) 

Issue fixed in wae4extensions

VERIFIED

This issue is closed automatically and wasn't rechecked in a current version of
OOo. The fixed issue should be integrated in OOo since more than half a year. If
you think this issue isn't fixed in a current version (OOo 3.1), please reopen
it and change the field 'Target Milestone' accordingly.

If you want to download a current version of OOo =>
http://download.openoffice.org/index.html
If you want to know more about the handling of fixed/verified issues =>
http://wiki.services.openoffice.org/wiki/Handle_fixed_verified_issues