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.
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.
Hi Joachim, On Tue, 2010-11-23 at 09:23 +0100, Joachim Trémouroux wrote: > - wrap current loadImage with something similar to this: > bool found = loadImage(...) > if (!found) { > found = loadImage( default_icon.png ...) > } > return found That sounds fine :-) I suggest we simply remove the boolean value to loadImage - since, in fact we will always return an image of some sort, and clean this up. Luckily the ImpImageTree thing is very much an internal VCL API - we should also move the 'impimagetree.hxx' header to make it a private internal VCL only header I think (ie. in inc/ not in inc/vcl) since it is not used outside, and update the backends. > 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? Sounds brilliant - though lets call it lc_missing_icon or something, to make the intention more clear (perhaps) :-) We should prolly audit all the call sites (5 or so?) that include & use the imagerepository.hxx header - and see if we want an extra parameter to that loadImage method to get the fallback image (or not). 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.
Hi Michael, Le 22 novembre 2010 22:07, Michael Meeks 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.
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
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.
2010/11/22 Michael Meeks : > 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 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