Issue 72766

Created attachment 41565 [details]
patch

Comments on size-cppuhelper.diff:

1  interfacecontainer.h #include <hash_map> can go (and #include <functional> as
well?).

2  interfacecontainer.hxx #define CONT_HASHMAP was bad&evil from the start.  I
would suggest leaving it alone (for max compatibility) and using the newly
introduced InterfaceMap typedef instead.

3  interfacecontainer.cxx findType and findLong are so similar they should be
turned into a single template (namely std::find).

> 1  interfacecontainer.h #include <hash_map> can go (and #include <functional> 
> as well?).

  You're braver than I ;-) what if someone was relying on this include,
somewhere ?  - either way, removed, cppuhelper builds itself at least.

> 2  interfacecontainer.hxx #define CONT_HASHMAP was bad&evil from the start.
> I would suggest leaving it alone (for max compatibility) and using the
> newly introduced InterfaceMap typedef instead.

  And you chicken out ? :-) http://go-oo.org/lxr/search?string=CONT_HASHMAP
appears to be used no-where across the code, base - so I removed it totally, is
that ok ? :-)

> 3  interfacecontainer.cxx findType and findLong are so similar they should
> be turned into a single template (namely std::find).

Are you certain you want to use std::find ? it looks somewhat painful to me
since the vector contains pairs, similarly std::find_if also sucks: I'd have to
create 2 Predicate classes with a member, constructor, bool operator etc. to
achieve the same effect. Unfortunately (it seems) the 'generic' approach is not
only more code, but way more confusing (after a 20minute read of the docs /
experiment) - that is unless I'm missing something (?) Or did you want me to
create my own templatised "find_first(?)" ? [ which I guess is doable ]

Created attachment 41605 [details]
patch (take 2)

The 1st hit instance of 1.) is vcl/source/helper/canvastools.cxx - worryingly
early in the build ...

2nd hit of 1.) is toolkit/inc/toolkit/controls/eventcontainer.hxx - I guess I'll
just re-attach the patch as/when I get a clean build with all the locations
fixed up [ that is assuming binfilter doesn't break (?) ].

Created attachment 41624 [details]
take 3 (with scattered hash_map fixups)

@2:  Yes, lets be bold.  :)

@3:  C++ is often a poor tool for functional things, and using the std
algorithms is often verbose and looks rather over-engineered, I agree.  (I
personally would have combined the three find functions in some way to be DRY, I
guess, with or without std algorithms, but no need to carry this on...)

committed to CWS cppuhelpshrink (still need to test a binfilter build really -
that is if it uses cppuhelper).

Stefan - you willing to be QA on the CWS ? :-)

binfilter is fine.

@mmeeks:  Are there any unit tests?  If yes, I could do QA.  ;)

> @mmeeks:  Are there any unit tests?  If yes, I could do QA.  ;)

Well, the smoketest passes nicely; and my OO.o has been running with it without
obvious problems for some time. Also, the functionality is mercifully somewhat
simple :-)

I was hoping that since this is entirely UI unrelated you could do the QA by
simple peer code review. Or do you have some better suggestion ?

@mmeeks:  Exactly because "this is entirely UI unrelated" and I guess the
functionality can be unit tested rather easily I was hoping for you to write
some tests (similar to cppuhelper/qa/propertysetmixin/, only much simpler as no
UNOIDL and no Java would be involved)---I do not trust my code review
capabilities so much that I would not feel more comfortable with a good
additional unit test in place...

Looks like that everyone wants this to have in 2.2. Michael: can you spend some time on writing these 
tests and make it into 2.2?

Pasting from 68929 (posted there in error):

In fact unit tests have been written, and (as expected since the patch is
remarkably simple) pass with no problems. [ sent by private mail to Stefan ].

The problem is that they use some ancient, ugly (but actually functional) unit
testing framework [ as opposed, to the new, trendy, (undocumented), and
(apparently) non-functional-out-of-the-box new unit testing framework ].

Thankfully someone kindly sent me the non-public docs on writing C++ unit tests
and I need to dive into this morass and work out why there is such compound
brokenness here; first to get the unit testing framework actually working for
me, and then (of course) to re-write the tests to use it. But as yet I have not
found the time or inclination.

Why, oh why - can I not just type "$ build test" or better "$ test" and get a
simple yes/no answer that works reliably, requires no extra installation /
download / build / documentation foo, and which I can extend easily ? The casual
writing of unit testage is drastically impeeded by the mess here :-)

Anyhow - it's good to vent some spleen; it's a curious decision not to take the
change IMHO, it's obviously correct and saves a noticable chunk of memory.

And my response from issue 68929:  "Sorry, I did not interpret the private mail
you sent me (and which I have discarded by now) as containing working unit tests
(you asked a number of questions in that mail), so I did not run them and
instead waited for you to come up with something working.  That's why I set CWS
cppuhelpshrink back to new and added a comment that I am waiting for unit tests."

Michael,

Thorsten was so kind to write me a bug for executing unit tests during building, see

http://udk.openoffice.org/issues/show_bug.cgi?id=71335

ok, so I ported my unit tests to cppunit, and I also ported the obsolete tests
from the broken/old rtl test framework, to the new CPPUNIT test framework -
making them cleaner, more readable & smaller, while still functioning.

Committed to the CWS - back to you Stefan for QA.

@mmeeks:  The gist of this issue's changes was to replace hash_maps with vectors
and introduce three helper find functions.  However, the committed unit tests
never call any of those find functions?

> However, the committed unit tests never call any of those find functions?

Committed. Your Thoroghness is laudable; you inspire me to new heights of
curiosity about all sorts of other changes.

Did I mention that an OO.o compiled with this also happens to work flawlessly,
and that the yet-more-unit tests (as expected) reveal no problems whatsoever (as
a trivial inspection of the patch shows) ?

Update:  Currently doing an incompatible build of this (unxsoli4.pro SRC680m197
+ cppuhelpshrink), got breaks due to now-missing includes in connectivity,
fpicker, sfx2, linguistic, and binfilter until now.  Once that is through
(before OOo 2.2 code freeze) I will nominate the CWS, but it is up to others to
decide whether it is actually included in 2.2 or shifted to 2.3.

Thanks Stefan, it's interesting to hear of more breakage (in excess of the take
#3 patch ?), we don't see that here in our 2.1 builds, but I guess I only
committed the cppuhelper part of the change to the CWS in error.

"in excess of the take #3 patch ?"  Yes, I took everything (I hope) tagged
cws_src680_cppuhelpshrink, which looked pretty much identical to that patch.

great; thanks :-)

.

.