Re: [Libreoffice] Replace (Byte|Uni|Xub_)String with O(U)String: Fill() method is missing
On 11/02/2011 05:21 PM, Michael Meeks wrote: The benefits of the stable ABI requirement are somewhat unclear to me. Mostly for the benefit of (external) client code. Also, as a secondary effect, striving for a stable API probably tends to make the authors of the API work harder to produce good quality (documentation, minimalism, orthogonality, ...; at least, that's my personal experience). If (as seems ~certain) we are going to have a flag day at some stage, there is no harm moving this little lot into the sal/ library. To what Sorry, don't get what you mean with flag day here. So - I think this splitting code into lots of different places for ABI reasons can be re-considered. An API should strive for various, potentially competing qualities. Stability is one, discoverability is another. Finding a good balance here can be a delicate exercise, and there is no single right answer. The concept of additional, external convenience APIs is quite widespread. So is the wisdom of if in doubt, leave it out. Confer, for example, chapter 2 of Reddy's API design for C++ (MK, 2011). Those are quite general remarks, meant more as something to keep in the back of your mind while working on LO's stable URE API, than as litmus tests how to handle the comphelper/string.hxx under discussion here (for which the case-by-case approach already discussed by Caolán sounds reasonable). Stephan ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] Replace (Byte|Uni|Xub_)String with O(U)String: Fill() method is missing
On 11/01/2011 07:50 PM, Eike Rathke wrote: Wouldn't they benefit/suffer from the stable ABI requirement then, whereas in comphelper they don't? Or am I mislead? Yup, that was the original rationale for introducing comphelper/string.hxx, see the comment at the start of the file. Stephan ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] Replace (Byte|Uni|Xub_)String with O(U)String: Fill() method is missing
On Wed, 2011-11-02 at 15:47 +0100, Stephan Bergmann wrote: On 11/01/2011 07:50 PM, Eike Rathke wrote: Wouldn't they benefit/suffer from the stable ABI requirement then, whereas in comphelper they don't? Or am I mislead? Yup, that was the original rationale for introducing comphelper/string.hxx, see the comment at the start of the file. The benefits of the stable ABI requirement are somewhat unclear to me. If (as seems ~certain) we are going to have a flag day at some stage, there is no harm moving this little lot into the sal/ library. To what degree these helpers randomly change over time is something that can presumably be quite easily seen from the git log. To me they look rather sensibly designed stable from the header [ just hard to find ]. So - I think this splitting code into lots of different places for ABI reasons can be re-considered. HTH, Michael. -- michael.me...@suse.com , Pseudo Engineer, itinerant idiot ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] Replace (Byte|Uni|Xub_)String with O(U)String: Fill() method is missing
On Mon, 2011-10-31 at 09:15 +0100, Stephan Bergmann wrote: However, I would also be sceptical that there are that many places that actually need to construct such an (immutable!) string---maybe at least some of the places are misguided attempts at preallocating some buffer of a given length? Those should then be rewritten to use rtl::OUStringBuffer instead. Worth noting that the ByteString Fill and Expand are now removed. So its just String that retains Fill/Expand. I handled the ByteString::Fill generally on a case-by-case basis. e.g. 565587eba10a8c4e01d4346669327bd9065c5dfa where the idea was simply to write 512 0's to disk Generally IIRC StringBuffers were the way to go. You can find padToLength in comphelper/inc/string.hxx which expands a buffer to the desired length and pads it with the requested character, which is pretty much an equivalent to the Expand concept anyway. C. ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] Replace (Byte|Uni|Xub_)String with O(U)String: Fill() method is missing
On Tue, 2011-11-01 at 11:01 +, Caolán McNamara wrote: Generally IIRC StringBuffers were the way to go. You can find padToLength in comphelper/inc/string.hxx which expands a buffer to the desired length and pads it with the requested character, which is pretty much an equivalent to the Expand concept anyway. Which makes me wonder whether we should be moving comphelper's string.hxx methods into sal/ anyway - they look reasonably sensible seemingly (though the rtl_uString_alloc stuff looks like it should be done with a StringBuffer instead in each instance). Since they're mostly documented - are there some file-able easy hacks there ? Thanks, Michael. -- michael.me...@suse.com , Pseudo Engineer, itinerant idiot ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] Replace (Byte|Uni|Xub_)String with O(U)String: Fill() method is missing
On Tue, 2011-11-01 at 11:58 +, Michael Meeks wrote: Which makes me wonder whether we should be moving comphelper's string.hxx methods into sal/ anyway - they look reasonably sensible seemingly case-by-case basis generally. e.g. methinks now that NaturalStringSorter should go into i18nutil or there abouts, definitely not sal anyway. Some of them are just bridges for the old string class, like the pad stuff and getToken so they probably can stay out. matchL and search/replace are possible contenders. (though the rtl_uString_alloc stuff looks like it should be done with a StringBuffer instead in each instance). The foo_alloc stuff is moved in there from a somewhat confused-in-usage family of overlapping stuff found in i18npool/i18nutil. StringBuffer's ctor calls rtl_string_new_WithLength, comment on rtl_string_alloc describes how it differ's from that, e.g. no memset on contents. Using a StringBuffer puts you at something of an extra remove over the private pData in order to tweak its length. Didn't really fancy fighting the i18npool/i18nutil usages. C. ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] Replace (Byte|Uni|Xub_)String with O(U)String: Fill() method is missing
On 10/29/2011 10:18 PM, Chr. Rossmanith wrote: the deprecated string classes have a Fill(n,c) method which fills a string with a single character c repeated n times. I found ~50 occurrences of Fill() using git grep, so it might be worth to add a Fill() method to OUString. Maybe the name of the new method could be a bit more verbose e.g. fillWithChar(). Probably I would have to add something similar to OUString fillWithChar(sal_uInt32 nNumber,|sal_Unicode cChar=' '|) to ustring.hxx and something like rtl_ustr_fillWithChar() to ustring.c| This functionality IMO only makes sense as a constructor (esp. for immutable OUString), like rtl::OUString::OUString(sal_Unicode value, sal_Int32 count); and probably some underlying C function like void rtl_uString_newFromValueAndCount( rtl_uString **, sal_Unicode value, sal_Int32 count); However, I would also be sceptical that there are that many places that actually need to construct such an (immutable!) string---maybe at least some of the places are misguided attempts at preallocating some buffer of a given length? Those should then be rewritten to use rtl::OUStringBuffer instead. Stephan ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] Replace (Byte|Uni|Xub_)String with O(U)String: Fill() method is missing
Hi Christina, Great to see you here again :-) On Sat, 2011-10-29 at 22:18 +0200, Chr. Rossmanith wrote: the deprecated string classes have a Fill(n,c) method which fills a string with a single character c repeated n times. I found ~50 occurrences of Fill() using git grep, so it might be worth to add a Fill() method to OUString. Maybe the name of the new method could be a bit more verbose e.g. fillWithChar(). As Stephan says, most of these seem to be reserving a string that is about to be operated on, and should really be using a string-buffer of a given size. Perhaps adding a method (as you suggest) to rtl::OUStringBuffer would make sense though. Obviously I need some assistance...any hint is welcome. Sounds like you're on track to me :-) I'd suggest hacking out the Fill method in UniString and incrementally fixing each of the sites that use it to use an rtl::OUStringBuffer that is pre-allocated, and if it seems that we do need to fill with ' ' or 0 (as a number of those seem to be), then presumably it'd make sense to add the method there. Does that make sense ? thanks for cleaning this up :-) All the best, Michael. -- michael.me...@suse.com , Pseudo Engineer, itinerant idiot ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
[Libreoffice] Replace (Byte|Uni|Xub_)String with O(U)String: Fill() method is missing
Hi, the deprecated string classes have a Fill(n,c) method which fills a string with a single character c repeated n times. I found ~50 occurrences of Fill() using git grep, so it might be worth to add a Fill() method to OUString. Maybe the name of the new method could be a bit more verbose e.g. fillWithChar(). Probably I would have to add something similar to OUString fillWithChar(sal_uInt32 nNumber,|sal_Unicode cChar=' '|) to ustring.hxx and something like rtl_ustr_fillWithChar() to ustring.c| |Obviously I need some assistance...any hint is welcome. Christina ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
