Re: [Libreoffice] [patch] binfilter: cleanup and compilation warnings removal.
Hi Michael,
Le 22 novembre 2010 22:07, Michael Meeks michael.me...@novell.com a écrit
:
Really, of course - I'd love to have someone working on eg.
http://wiki.documentfoundation.org/Development/Easy_Hacks#don.27t_ship_150_duplicate_placeholder_icons
Which should be mind-numblingly simple and yet yield a real
image-size
(and hence performance) win :-)
Any chance of a small detour on the way ? :-)
Thanks anyhow,
Michael.
--
michael.me...@novell.com , Pseudo Engineer, itinerant idiot
I will work on this. I see two possible ways:
- wrap current loadImage with something similar to this:
bool found = loadImage(...)
if (!found) {
found = loadImage( default_icon.png ...)
}
return found
- alternatively, add the default icon path to the list of paths that are
passed to the ImplImageTree::find method.
I think this will be a bit less efficient as the normal case is that the
icon exist. Your opinion?
Furthermore, the duplicate icons currently exist in several (4?) sizes. So
we could have 5 different icons:
lc_default_icon.png
lx_default_icon.png
sc_default_icon.png
sx_default_icon.png
default_icon.png
and based on the input name we can return an icon of the correct size.
Does it look ok for you?
Regards,
Joachim.
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [patch] binfilter: cleanup and compilation warnings removal.
On 11/23/2010 09:23 AM, Joachim Trémouroux wrote: Hi Michael, I will work on this. I see two possible ways: Ok Michael, then I will continue within binfilter... ::-)) But a additional question: From Norbert Thiebaud: To be consistent, I rather see them commented out i.e foo(int bar) - foo(int /*bar*/) rather than foo(int) To have a better code, would it not be better to change the prototyping of the function from foo (int) to foo() ?? Or did I miss a point? So would the code be cleaner or do you intend to drop the binfilter components very soon ? regards ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [patch] binfilter: cleanup and compilation warnings removal.
On Tue, 2010-11-23 at 21:05 +0100, Pierre-André Jacquod wrote: To have a better code, would it not be better to change the prototyping of the function from foo (int) to foo() ?? Or did I miss a point? It would be better, *but* often these are virtual methods and have to retain their signature to remain virtual, not always, but sometimes and it can be tricky to get right. For the case of the binfilter its just easier to guarantee lockin the current behaviour. C. ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [patch] binfilter: cleanup and compilation warnings removal.
Hi Joachim, On Mon, 2010-11-22 at 13:59 +0100, Joachim Trémouroux wrote: I have fixed some compilation warnings in binfilter and some unnecessary comments. Patch is attached for review. Hokay - the binfilter is of course un-loved and under-maintained. I have two questions: - /*N*/ . Should I fix them massively? I have currently left them unchanged to avoid a big cleanup patch. I guess this should be done by some large-scale sed at some point centrally. Certainly not a useful patch for the list ;-) - Unused parameters in some methods are triggering compilation warnings. Should I fix them by removing the parameter name ? Sounds reasonable. Really though - the binfilter is not the best place to focus cleanups (though I appreciate it is fugly old code ;-) Most people will not want it (only useful for decade-old binary StarOffice file formats). Thanks, Michael. -- michael.me...@novell.com , Pseudo Engineer, itinerant idiot ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [patch] binfilter: cleanup and compilation warnings removal.
2010/11/22 Michael Meeks michael.me...@novell.com: Hi Joachim, On Mon, 2010-11-22 at 13:59 +0100, Joachim Trémouroux wrote: I have fixed some compilation warnings in binfilter and some unnecessary comments. Patch is attached for review. Hokay - the binfilter is of course un-loved and under-maintained. I have two questions: - /*N*/ . Should I fix them massively? I have currently left them unchanged to avoid a big cleanup patch. I guess this should be done by some large-scale sed at some point centrally. Certainly not a useful patch for the list ;-) - Unused parameters in some methods are triggering compilation warnings. Should I fix them by removing the parameter name ? Sounds reasonable. To be consistent, I rather see them commented out i.e foo(int bar) - foo(int /*bar*/) rather than foo(int) Really though - the binfilter is not the best place to focus cleanups (though I appreciate it is fugly old code ;-) Most people will not want it (only useful for decade-old binary StarOffice file formats). as unloved as it maybe. as long as it _IS_ in the build, it has to build and, especially in // build, having the build.log flooded with thousands of warning kind of defeat the point of having warning in the first place. Maybe you could draw the outline of what need to be accomplish to make binfilter a separate stand-alone project ? Norbert Thanks, Michael. -- michael.me...@novell.com , Pseudo Engineer, itinerant idiot ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [patch] binfilter: cleanup and compilation warnings removal.
Hi,
- Unused parameters in some methods are triggering compilation
warnings. Should I fix them by removing the parameter name ?
Sounds reasonable.
Really though - the binfilter is not the best place to focus cleanups
(though I appreciate it is fugly old code ;-) Most people will not want
it (only useful for decade-old binary StarOffice file formats).
Okay, but at compile time, there is really a flow of warning. So I
wanted also to work a bit on it. That's just cleaning, not really
improving.
But when I see:
clone/filters/binfilter/bf_sc/source/core/tool/sc_token.cxx:1573:28:
warning: comparison is always false due to limited range of data type
and the source code is (it starts form line 1573..
if( n MAXSTRLEN-1 )
/*N*/ {
/*?*/ DBG_ERRORFILE( bad string array boundary );
/*?*/ USHORT nDiff = n - (MAXSTRLEN-1);
/*?*/ n = MAXSTRLEN-1;
/*?*/ rStream.Read( c, n );
/*?*/ rStream.SeekRel( nDiff );
/*N*/ }
/*N*/ else
/*N*/ rStream.Read( c, n );
/*N*/ cStr[ n ] = 0;
would as cleaning be acceptable to change the code to:
/*N*/ rStream.Read( c, n );
/*N*/ cStr[ n ] = 0;
??? I wonder...
If yes, a lot of code can be removed (between 40 and 60 comparisons are
generating this warning). So I would prefer not starting and beeing told
not to touch it afterward.
Thanks
Pierre-André
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [patch] binfilter: cleanup and compilation warnings removal.
Hi Pierre, On Mon, 2010-11-22 at 21:06 +0100, Pierre-André Jacquod wrote: Okay, but at compile time, there is really a flow of warning. So I wanted also to work a bit on it. That's just cleaning, not really improving. Sure sure :-) fair enough. clone/filters/binfilter/bf_sc/source/core/tool/sc_token.cxx:1573:28: warning: comparison is always false due to limited range of data type Right - so we can chop that code out with prejudice as you say :-) would as cleaning be acceptable to change the code to: /*N*/ rStream.Read( c, n ); /*N*/ cStr[ n ] = 0; Of course ! no point in keeping dead code around. If yes, a lot of code can be removed (between 40 and 60 comparisons are generating this warning). So I would prefer not starting and beeing told not to touch it afterward. Go for it :-) of course, this is the sort of patch that scares patch reviewers, so I would pair up the compile messages of this form, with the diff (if you can) ? Really, of course - I'd love to have someone working on eg. http://wiki.documentfoundation.org/Development/Easy_Hacks#don.27t_ship_150_duplicate_placeholder_icons Which should be mind-numblingly simple and yet yield a real image-size (and hence performance) win :-) Any chance of a small detour on the way ? :-) Thanks anyhow, Michael. -- michael.me...@novell.com , Pseudo Engineer, itinerant idiot ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
