Re: [Libreoffice] [PUSHED] Re: [PATCH 02/12] RTL_CONSTASCII_USTRINGPARAM in components cui options
Hi Wol, On 2010-11-23 at 20:49 +, Wols Lists wrote: > > String aFoo = String::createFromAscii( "Something" ) > > > > should be replaced, with > > > > String aFoo( RTL_CONSTASCII_USTRINGPARAM( "Something" ) ) > > Fine. Fixes in base will appear as I spot them :-) Just one important > little point - the reason I asked is that I think the strings might > actually be being passed to a non-OOo library - will this break a > third-party library? (My C++-foo isn't good enough to answer this > question for myself :-) This 'String' is an LibO internal class too (usually called "tools string"), so should be no problem. Or do you mean something different? Regards, Kendy ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [PUSHED] Re: [PATCH 02/12] RTL_CONSTASCII_USTRINGPARAM in components cui options
On 23/11/10 19:00, Jan Holesovsky wrote: > Hi Wol, Kevin, all, > > On 2010-11-21 at 23:48 -0500, Kevin Hunter wrote: > >>> Forgive what might be a stupid question, but I've seen >>> >>> String::createFromAscii >>> >>> Will version 2 find those, and should they be replaced? >> Not a stupid question at all. Regular expressions aren't the most >> transparent of creatures. >> >> As I wrote the regex, round 2 /will/ find those. Since I don't know if >> those should be replaced, I assume that they shouldn't be, making this >> "an edge case". The correct procedure then would be a "Replace and >> Find" as opposed to "Replace All". This way one inspects every change >> rather than blindly updating every occurrence. > Yes, even String::createFromAscii() usage should be replaced the similar > way, the UniString class (that is the class aliased as String) has the > appropriate constructor, ie. > > String aFoo = String::createFromAscii( "Something" ) > > should be replaced, with > > String aFoo( RTL_CONSTASCII_USTRINGPARAM( "Something" ) ) > > Regards, > Kendy Fine. Fixes in base will appear as I spot them :-) Just one important little point - the reason I asked is that I think the strings might actually be being passed to a non-OOo library - will this break a third-party library? (My C++-foo isn't good enough to answer this question for myself :-) Cheers, Wol ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [PUSHED] Re: [PATCH 02/12] RTL_CONSTASCII_USTRINGPARAM in components cui options
Hi Wol, Kevin, all, On 2010-11-21 at 23:48 -0500, Kevin Hunter wrote: > > Forgive what might be a stupid question, but I've seen > > > > String::createFromAscii > > > > Will version 2 find those, and should they be replaced? > > Not a stupid question at all. Regular expressions aren't the most > transparent of creatures. > > As I wrote the regex, round 2 /will/ find those. Since I don't know if > those should be replaced, I assume that they shouldn't be, making this > "an edge case". The correct procedure then would be a "Replace and > Find" as opposed to "Replace All". This way one inspects every change > rather than blindly updating every occurrence. Yes, even String::createFromAscii() usage should be replaced the similar way, the UniString class (that is the class aliased as String) has the appropriate constructor, ie. String aFoo = String::createFromAscii( "Something" ) should be replaced, with String aFoo( RTL_CONSTASCII_USTRINGPARAM( "Something" ) ) Regards, Kendy ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [PUSHED] Re: [PATCH 02/12] RTL_CONSTASCII_USTRINGPARAM in components cui options
At 4:58pm -0500 Sun, 21 Nov 2010, Wols Lists wrote: On 18/11/10 13:36, Kevin Hunter wrote: As I assume you're using a regex, you might consider catching this by doing the search and replace in series. Here's an example: 1. Catch the 'OUString +?= ...createFromAscii...' case and replace with 'OUString var( RTL...)' search: OUString\s*\w+\s*\+?=\s*\S*createFromAscii\(\s*"([^"]*)"\s*\) replace: $1 $2( RTL_CONSTASCII_USTRINGPARAM( "$4" )) 2. Then go back for a second pass with something like this: search: ::createFromAscii\(\s*"([^"]*)"\s*\) replace: $1 $2( RTL_CONSTASCII_USTRINGPARAM( "$4" )) The solution isn't perfect, as it still misses certain edge cases, but should at least help a little bit. Forgive what might be a stupid question, but I've seen String::createFromAscii Will version 2 find those, and should they be replaced? Not a stupid question at all. Regular expressions aren't the most transparent of creatures. As I wrote the regex, round 2 /will/ find those. Since I don't know if those should be replaced, I assume that they shouldn't be, making this "an edge case". The correct procedure then would be a "Replace and Find" as opposed to "Replace All". This way one inspects every change rather than blindly updating every occurrence. Thanks for asking! Kevin P.S. As an aside, I'll make a minor plug that learning regular expressions makes possible or less mundane *many* tasks in the programming or text processing world, and many editors have regular expression support built-in. Vim, Emacs, jEdit, and Kate (among others) have them by default, while gEdit (among others) has them available via a plugin. If one doesn't know about them, one might be interested in perusing http://www.regular-expressions.info/, or just generically googling. ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [PUSHED] Re: [PATCH 02/12] RTL_CONSTASCII_USTRINGPARAM in components cui options
On 18/11/10 13:36, Kevin Hunter wrote: > As I assume you're using a regex, you might consider catching this by > doing the search and replace in series. Here's an example: > > 1. Catch the 'OUString +?= ...createFromAscii...' case and replace > with 'OUString var( RTL...)' > > search: OUString\s*\w+\s*\+?=\s*\S*createFromAscii\(\s*"([^"]*)"\s*\) > replace: $1 $2( RTL_CONSTASCII_USTRINGPARAM( "$4" )) > > 2. Then go back for a second pass with something like this: > > search: ::createFromAscii\(\s*"([^"]*)"\s*\) > replace: $1 $2( RTL_CONSTASCII_USTRINGPARAM( "$4" )) > > The solution isn't perfect, as it still misses certain edge cases, but > should at least help a little bit. > Forgive what might be a stupid question, but I've seen String::createFromAscii Will version 2 find those, and should they be replaced? Cheers, Wol ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [PUSHED] Re: [PATCH 02/12] RTL_CONSTASCII_USTRINGPARAM in components cui options
As I assume you're using a regex, you might consider catching this by
doing the search and replace in series. Here's an example:
1. Catch the 'OUString +?= ...createFromAscii...' case and replace with
'OUString var( RTL...)'
search: OUString\s*\w+\s*\+?=\s*\S*createFromAscii\(\s*"([^"]*)"\s*\)
replace: $1 $2( RTL_CONSTASCII_USTRINGPARAM( "$4" ))
2. Then go back for a second pass with something like this:
search: ::createFromAscii\(\s*"([^"]*)"\s*\)
replace: $1 $2( RTL_CONSTASCII_USTRINGPARAM( "$4" ))
The solution isn't perfect, as it still misses certain edge cases, but
should at least help a little bit.
Cheers,
Kevin
At 3:51pm -0500 Wed, 17 Nov 2010, Pierre-André Jacquod wrote:
Sharp eyes.. Just to keep you trainded..:-(
No really sorry, Despite reviewing diff, I did not catch this one.
Will take more care
On 11/17/2010 05:18 PM, Caolán McNamara wrote:
On Tue, 2010-11-16 at 22:39 +0100, Pierre-André Jacquod wrote:
On 11/16/2010 10:37 PM, Pierre-André Jacquod wrote:
Hello,
being off for some days, here the collection of patches I produced in
between.
Mostly good, but careful here, see...
-aAutoStr += ::rtl::OUString::createFromAscii( " (" );
+aAutoStr += ::rtl::OUString(RTL_CONSTASCII_USTRINGPARAM("(") );
you changed the string by accident from a bracket with a preceding space
to one with no preceding space, clearly what's between "" has to remain
the same :-). Fixed that typo and the rest looks good, pushed.
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [PUSHED] Re: [PATCH 02/12] RTL_CONSTASCII_USTRINGPARAM in components cui options
Hello,
Sharp eyes.. Just to keep you trainded..:-(
No really sorry, Despite reviewing diff, I did not catch this one.
Will take more care
On 11/17/2010 05:18 PM, Caolán McNamara wrote:
> On Tue, 2010-11-16 at 22:39 +0100, Pierre-André Jacquod wrote:
>> On 11/16/2010 10:37 PM, Pierre-André Jacquod wrote:
>>> Hello,
>>> being off for some days, here the collection of patches I produced in
>>> between.
>
> Mostly good, but careful here, see...
>
> -aAutoStr += ::rtl::OUString::createFromAscii( " (" );
> +aAutoStr += ::rtl::OUString(RTL_CONSTASCII_USTRINGPARAM("(") );
>
> you changed the string by accident from a bracket with a preceding space
> to one with no preceding space, clearly what's between "" has to remain
> the same :-). Fixed that typo and the rest looks good, pushed.
>
> C.
>
>
> ___
> 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
