Issue 72766 - save 700k from cppuhelper interfacecontainer ...
Summary: save 700k from cppuhelper interfacecontainer ...
Status: CLOSED FIXED
Alias: None
Product: udk
Classification: Code
Component: code (show other issues)
Version: 680m198
Hardware: All Linux, all
: P3 Trivial (vote)
Target Milestone: OOo 2.2
Assignee: Stephan Bergmann
QA Contact: issues@udk
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-12-19 16:50 UTC by mmeeks
Modified: 2007-01-30 12:04 UTC (History)
3 users (show)

See Also:
Issue Type: PATCH
Latest Confirmation in: ---
Developer Difficulty: ---


Attachments
patch (9.10 KB, patch)
2006-12-19 16:50 UTC, mmeeks
no flags Details | Diff
patch (take 2) (12.12 KB, patch)
2006-12-20 16:10 UTC, mmeeks
no flags Details | Diff
take 3 (with scattered hash_map fixups) (15.29 KB, patch)
2006-12-21 13:29 UTC, mmeeks
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this issue.
Description mmeeks 2006-12-19 16:50:05 UTC
cf. mail on dev@

Before: 19264 19256 + 19268 + 3 / p : 19262
After:  18552 18552 + 18544 + 3 / p : 18549
Saving: - p                         :   713(k)

The Int32 container is ~always empty, and the Types container is:

count   num elements
    282 1
   2841 2
   1569 3
     38 4

and as expected switching to a vector has no measurable performance impact.
Comment 1 mmeeks 2006-12-19 16:50:22 UTC
Created attachment 41565 [details]
patch
Comment 2 Stephan Bergmann 2006-12-20 10:31:49 UTC
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).
Comment 3 mmeeks 2006-12-20 16:06:41 UTC
> 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 ]
Comment 4 mmeeks 2006-12-20 16:10:30 UTC
Created attachment 41605 [details]
patch (take 2)
Comment 5 mmeeks 2006-12-20 17:49:39 UTC
The 1st hit instance of 1.) is vcl/source/helper/canvastools.cxx - worryingly
early in the build ...
Comment 6 mmeeks 2006-12-21 09:43:46 UTC
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 (?) ].
Comment 7 mmeeks 2006-12-21 13:29:59 UTC
Created attachment 41624 [details]
take 3 (with scattered hash_map fixups)
Comment 8 Stephan Bergmann 2006-12-22 09:23:42 UTC
@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...)
Comment 9 mmeeks 2006-12-22 14:54:00 UTC
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 ? :-)
Comment 10 mmeeks 2006-12-22 17:50:23 UTC
binfilter is fine.
Comment 11 Stephan Bergmann 2007-01-02 08:04:01 UTC
@mmeeks:  Are there any unit tests?  If yes, I could do QA.  ;)
Comment 12 mmeeks 2007-01-02 14:54:18 UTC
> @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 ?
Comment 13 Stephan Bergmann 2007-01-02 15:45:46 UTC
@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...
Comment 14 pavel 2007-01-16 20:49:20 UTC
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?
Comment 15 mmeeks 2007-01-16 21:00:04 UTC
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.
Comment 16 Stephan Bergmann 2007-01-17 08:41:42 UTC
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."
Comment 17 kay.ramme 2007-01-17 12:12:44 UTC
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

Comment 18 mmeeks 2007-01-22 12:25:34 UTC
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.
Comment 19 Stephan Bergmann 2007-01-22 15:21:58 UTC
@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?
Comment 20 mmeeks 2007-01-22 20:40:49 UTC
> 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) ?
Comment 21 Stephan Bergmann 2007-01-24 09:45:11 UTC
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.
Comment 22 mmeeks 2007-01-24 10:05:49 UTC
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.
Comment 23 Stephan Bergmann 2007-01-24 10:26:35 UTC
"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.
Comment 24 mmeeks 2007-01-24 12:13:57 UTC
great; thanks :-)
Comment 25 Stephan Bergmann 2007-01-25 13:20:29 UTC
.
Comment 26 Stephan Bergmann 2007-01-30 12:04:19 UTC
.