You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@stdcxx.apache.org by Liviu Nicoara <ni...@hates.ms> on 2012/10/10 14:25:08 UTC
[PATCH] STDCXX-1073
2012-10-10 Liviu Nicoara <ln...@apache.org>
* src/collate.cpp (__rw_strnxfrm): preserved embedded NULs
Re: [PATCH] STDCXX-1073
Posted by Liviu Nicoara <ni...@hates.ms>.
On 10/25/12 11:31, Martin Sebor wrote:
> There are comments suggesting that calling do_transform()
> on the whole string may be suboptimal. Intuitively it makes
> sense that calling wcscoll() (in a loop, on the NUL-terminated
> substrings, if necessary) should be faster than simply calling
> do_transform() followed by wstring::compare(), but it would
> make sense to confirm the hypothesis before implementing the
> optimization.
Alright, same files, two more patches.
2012-10-28 Liviu Nicoara <ln...@apache.org>
Fixes to collate facet and test enhancements:
* src/collate.cpp (__rw_strnxfrm): corrected processing of
embedded NULs. (__rw_wcsnxfrm) same (duplicated code).
(collate_byname<wchar_t>::do_compare): fixed string comparison
return values, re-implemented the wcscoll-based comparison.
* tests/localization/22.locale.collate.cpp: implemented a simpler
collation test for strings with embedded NULs.
All existing and new collate tests pass, but I think some more test enhancements are required.
Liviu
Re: [PATCH] STDCXX-1073
Posted by Martin Sebor <ms...@gmail.com>.
On 10/25/2012 06:22 AM, Liviu Nicoara wrote:
> On 10/24/12 11:11, Martin Sebor wrote:
>> On 10/24/2012 07:12 AM, Liviu Nicoara wrote:
>>> [...]
>>> I modified the test according to the suggestions. The test fails all
>>> corresponding wide-char cases and I am investigating other potential
>>> defects as well. For example, I do not think that employing strcoll and
>>> wcscoll in compare is correct as they stop at the first NUL, although
>>> strings may contain characters after the NUL that alter the result of
>>> the comparison.
>>
>> I would expect the wchar_t specialization to be analogous
>> to the narrow one. In fact (without looking at the code),
>> I would even think both could be implemented in terms of
>> the same function template specialized on the character
>> type and on the libc string function. (Although I'm not
>> necessarily suggesting this as the solution to this issue.)
>>
>
> They are not similar, AFAICT. In revision 367462 you implemented the
> wide byname do_compare in terms of wcscoll, if available, when using
> libc. That implementation does not take into account embedded NULs. This
> is in contrast with the narrow byname which simply transforms the
> strings and compares them.
IIUC, rev 367462 actually implements the function in terms of
do_transform() when wcscoll() isn't available as a workaround.
Ironically, the workaround is actually better than the default
implementation.
>
> Testing shows that an implementation of wide byname do_compare identical
> with the narrow version passes all tests, as expected.
>
> It's easy to me to just remove the wcscoll-based implementation. Also,
> error reporting from wcscoll seems difficult to use when libc does not
> have thread-local errno's, and right now we don't check for errors.
> OTOH, I expect wcscoll to be faster than the (simpler) transformation
> followed by comparison.
>
> I incline towards the simpler approach. Thoughts?
There are comments suggesting that calling do_transform()
on the whole string may be suboptimal. Intuitively it makes
sense that calling wcscoll() (in a loop, on the NUL-terminated
substrings, if necessary) should be faster than simply calling
do_transform() followed by wstring::compare(), but it would
make sense to confirm the hypothesis before implementing the
optimization.
Martin
>
> Liviu
Re: [PATCH] STDCXX-1073
Posted by Liviu Nicoara <ni...@hates.ms>.
On 10/24/12 11:11, Martin Sebor wrote:
> On 10/24/2012 07:12 AM, Liviu Nicoara wrote:
>> [...]
>> I modified the test according to the suggestions. The test fails all
>> corresponding wide-char cases and I am investigating other potential
>> defects as well. For example, I do not think that employing strcoll and
>> wcscoll in compare is correct as they stop at the first NUL, although
>> strings may contain characters after the NUL that alter the result of
>> the comparison.
>
> I would expect the wchar_t specialization to be analogous
> to the narrow one. In fact (without looking at the code),
> I would even think both could be implemented in terms of
> the same function template specialized on the character
> type and on the libc string function. (Although I'm not
> necessarily suggesting this as the solution to this issue.)
>
They are not similar, AFAICT. In revision 367462 you implemented the
wide byname do_compare in terms of wcscoll, if available, when using
libc. That implementation does not take into account embedded NULs. This
is in contrast with the narrow byname which simply transforms the
strings and compares them.
Testing shows that an implementation of wide byname do_compare identical
with the narrow version passes all tests, as expected.
It's easy to me to just remove the wcscoll-based implementation. Also,
error reporting from wcscoll seems difficult to use when libc does not
have thread-local errno's, and right now we don't check for errors.
OTOH, I expect wcscoll to be faster than the (simpler) transformation
followed by comparison.
I incline towards the simpler approach. Thoughts?
Liviu
Re: [PATCH] STDCXX-1073
Posted by Martin Sebor <ms...@gmail.com>.
On 10/24/2012 07:12 AM, Liviu Nicoara wrote:
> A few observations after spending a few hours working on the improved test.
>
> On 10/21/12 19:08, Martin Sebor wrote:
>> ...
>> There's no requirement that embedded NULs must be preserved
>> (that's just how it happens to be implemented). I find it best
>> to avoid relying on the knowledge of implementation details
>> when exercising the library so that tests don't start failing
>> after a conforming optimization or some such tweak is added
>> to the code.
>>
>
> I agree with the implementation details part. However, there is only one
> way NULs get processed and that is by keeping them in the string,
> verbatim. In this respect the previous test incarnation was a stronger
> test.
Consider an encoding where UCHAR_MAX doesn't correspond
to a valid character, and a hypothetical implementation that,
before transforming the string, increments the value of each
character by 1. That will lose all the initial embedded NULs.
>
> I modified the test according to the suggestions. The test fails all
> corresponding wide-char cases and I am investigating other potential
> defects as well. For example, I do not think that employing strcoll and
> wcscoll in compare is correct as they stop at the first NUL, although
> strings may contain characters after the NUL that alter the result of
> the comparison.
I would expect the wchar_t specialization to be analogous
to the narrow one. In fact (without looking at the code),
I would even think both could be implemented in terms of
the same function template specialized on the character
type and on the libc string function. (Although I'm not
necessarily suggesting this as the solution to this issue.)
>
>> Attached is a test case that reproduces the bug without relying
>> on implementation details. The test passes with your patch,
>> confirming that it's good. This test still makes the assumption
>> that strings that lexicographically distinct strings compare
>> unequal in the en_US locale, but I think that's a safe assumption
>> regardless of the encoding (though only for the hardcoded strings).
>
> All narrow test cases pass fine, but I gotta look into the wide-char
> failures.
Good to hear! I wish I had more time to look into it with you.
Martin
>
> Liviu
>
Re: [PATCH] STDCXX-1073
Posted by Liviu Nicoara <ni...@hates.ms>.
A few observations after spending a few hours working on the improved test.
On 10/21/12 19:08, Martin Sebor wrote:
> ...
> There's no requirement that embedded NULs must be preserved
> (that's just how it happens to be implemented). I find it best
> to avoid relying on the knowledge of implementation details
> when exercising the library so that tests don't start failing
> after a conforming optimization or some such tweak is added
> to the code.
>
I agree with the implementation details part. However, there is only one
way NULs get processed and that is by keeping them in the string,
verbatim. In this respect the previous test incarnation was a stronger test.
I modified the test according to the suggestions. The test fails all
corresponding wide-char cases and I am investigating other potential
defects as well. For example, I do not think that employing strcoll and
wcscoll in compare is correct as they stop at the first NUL, although
strings may contain characters after the NUL that alter the result of
the comparison.
> Attached is a test case that reproduces the bug without relying
> on implementation details. The test passes with your patch,
> confirming that it's good. This test still makes the assumption
> that strings that lexicographically distinct strings compare
> unequal in the en_US locale, but I think that's a safe assumption
> regardless of the encoding (though only for the hardcoded strings).
All narrow test cases pass fine, but I gotta look into the wide-char
failures.
Liviu
Re: [PATCH] STDCXX-1073
Posted by Liviu Nicoara <ni...@hates.ms>.
On 10/21/12 19:08, Martin Sebor wrote:
> There's no requirement that embedded NULs must be preserved
> (that's just how it happens to be implemented). I find it best
> to avoid relying on the knowledge of implementation details
> when exercising the library so that tests don't start failing
> after a conforming optimization or some such tweak is added
> to the code.
That's right. I'll improve it.
Thanks for the review.
Liviu
Re: [PATCH] STDCXX-1073
Posted by Martin Sebor <ms...@gmail.com>.
The library patch seems correct but the test case in the issue
and the new test aren't strictly conforming. The only requirement
on collate::tranform() is that it return...
A basic_string<charT> value that, compared lexicographically
with the result of calling transform() on another string,
yields the same result as calling do_compare() on the same
two strings.
There's no requirement that embedded NULs must be preserved
(that's just how it happens to be implemented). I find it best
to avoid relying on the knowledge of implementation details
when exercising the library so that tests don't start failing
after a conforming optimization or some such tweak is added
to the code.
Attached is a test case that reproduces the bug without relying
on implementation details. The test passes with your patch,
confirming that it's good. This test still makes the assumption
that strings that lexicographically distinct strings compare
unequal in the en_US locale, but I think that's a safe assumption
regardless of the encoding (though only for the hardcoded strings).
Martin
On 10/18/2012 04:28 PM, Liviu Nicoara wrote:
> On 10/18/12 11:02, Martin Sebor wrote:
>> On 10/16/2012 10:38 AM, Liviu Nicoara wrote:
>>> I have enhanced the collation test, 22.locale.collate.cpp with a bunch
>>> of cases that deal with embedded strings, inside the input strings as
>>> well as at the edges. More defects became apparent, and they have been
>>> fixed.
>>>
>>> I have re-worked the src/collate.cpp patch and I am attaching it here.
>>> All collation tests (old and new) pass. If there are no objections, I
>>> will check it in later in the week.
>>
>> There are tabs in the patch (we need spaces). They also make
>> the patch harder to review than it otherwise would be. I would
>> also suggest keeping the MSVC < 8.0 block and cleaning things
>> up separately. That will also make the patch easier to review.
>> (I'm so swamped that this matters to me.)
>
> I looked at both (latest) patches and they do not have tabs in them.
> They were sent in my last post on the 16th, the one you replied to. The
> diff without -w is quite a bit verbose because I moved a whole block in
> both collate.cpp functions, inside an if, so there are whitespace
> differences. svn diff -w gave me a quite decent view of the changes
> without having to looks at the actual patched source file.
>
> I am re-attaching the patches, this time created without -w so they
> would be a bit verbose. No tabs this time either.
>
>>
>> I haven't looked at the test patch yet. If you could clean up
>> the tab issue and reduce the amount of unnecessary whitespace
>> changes I promise to review it this weekend.
>
> The test enhancements are pretty plain, just a bunch of added stuff.
>
> Thanks,
> Liviu
>
Re: [PATCH] STDCXX-1073
Posted by Liviu Nicoara <ni...@hates.ms>.
On 10/18/12 11:02, Martin Sebor wrote:
> On 10/16/2012 10:38 AM, Liviu Nicoara wrote:
>> I have enhanced the collation test, 22.locale.collate.cpp with a bunch
>> of cases that deal with embedded strings, inside the input strings as
>> well as at the edges. More defects became apparent, and they have been
>> fixed.
>>
>> I have re-worked the src/collate.cpp patch and I am attaching it here.
>> All collation tests (old and new) pass. If there are no objections, I
>> will check it in later in the week.
>
> There are tabs in the patch (we need spaces). They also make
> the patch harder to review than it otherwise would be. I would
> also suggest keeping the MSVC < 8.0 block and cleaning things
> up separately. That will also make the patch easier to review.
> (I'm so swamped that this matters to me.)
I looked at both (latest) patches and they do not have tabs in them. They were sent in my last post on the 16th, the one you replied to. The diff without -w is quite a bit verbose because I moved a whole block in both collate.cpp functions, inside an if, so there are whitespace differences. svn diff -w gave me a quite decent view of the changes without having to looks at the actual patched source file.
I am re-attaching the patches, this time created without -w so they would be a bit verbose. No tabs this time either.
>
> I haven't looked at the test patch yet. If you could clean up
> the tab issue and reduce the amount of unnecessary whitespace
> changes I promise to review it this weekend.
The test enhancements are pretty plain, just a bunch of added stuff.
Thanks,
Liviu
Re: [PATCH] STDCXX-1073
Posted by Martin Sebor <ms...@gmail.com>.
On 10/16/2012 10:38 AM, Liviu Nicoara wrote:
> I have enhanced the collation test, 22.locale.collate.cpp with a bunch
> of cases that deal with embedded strings, inside the input strings as
> well as at the edges. More defects became apparent, and they have been
> fixed.
>
> I have re-worked the src/collate.cpp patch and I am attaching it here.
> All collation tests (old and new) pass. If there are no objections, I
> will check it in later in the week.
There are tabs in the patch (we need spaces). They also make
the patch harder to review than it otherwise would be. I would
also suggest keeping the MSVC < 8.0 block and cleaning things
up separately. That will also make the patch easier to review.
(I'm so swamped that this matters to me.)
I haven't looked at the test patch yet. If you could clean up
the tab issue and reduce the amount of unnecessary whitespace
changes I promise to review it this weekend.
Martin
>
> Thanks,
> Liviu
>
> On 10/13/12 11:16, Liviu Nicoara wrote:
>> The initial patch does not pass the following test case. Have re-worked
>> the patch and attached it to the incident, and I am also attaching it
>> here. It passes all collate tests.
>
>
>
Re: [PATCH] STDCXX-1073
Posted by Liviu Nicoara <ni...@hates.ms>.
I have enhanced the collation test, 22.locale.collate.cpp with a bunch
of cases that deal with embedded strings, inside the input strings as
well as at the edges. More defects became apparent, and they have been
fixed.
I have re-worked the src/collate.cpp patch and I am attaching it here.
All collation tests (old and new) pass. If there are no objections, I
will check it in later in the week.
Thanks,
Liviu
On 10/13/12 11:16, Liviu Nicoara wrote:
> The initial patch does not pass the following test case. Have re-worked
> the patch and attached it to the incident, and I am also attaching it
> here. It passes all collate tests.
Re: [PATCH] STDCXX-1073
Posted by Liviu Nicoara <ni...@hates.ms>.
The initial patch does not pass the following test case. Have re-worked
the patch and attached it to the incident, and I am also attaching it
here. It passes all collate tests.
Here is the second test case:
$ cat ../../tests/localization/t.cpp; nice make t.cpp && ./t af_ZA.utf8;
echo $?
#include <iostream>
#include <locale>
#include <string>
int
main (int argc, char** argv)
{
std::locale loc (argv [1]);
const std::collate< char >& fac =
std::use_facet< std::collate< char > > (loc);
char const buf [] = "a\0\0b";
std::string s = fac.transform (buf, buf + sizeof buf - 1);
size_t i = 0;
for (; i < s.size () && 0 == s [i]; ++i) ;
return !(i == 2);
}
1
On 10/10/12 08:25, Liviu Nicoara wrote:
> 2012-10-10 Liviu Nicoara <ln...@apache.org>
>
> * src/collate.cpp (__rw_strnxfrm): preserved embedded NULs
>