Bug 13650 - string::compare should not (always) use traits_type::length()
Summary: string::compare should not (always) use traits_type::length()
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: libstdc++ (show other bugs)
Version: unknown
: P2 normal
Target Milestone: 3.3.3
Assignee: Paolo Carlini
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2004-01-12 04:13 UTC by Jacques Labuschagne
Modified: 2004-01-25 13:14 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2004-01-12 05:39:52


Attachments
Proposed resolution. (1.03 KB, patch)
2004-01-12 08:01 UTC, Jacques Labuschagne
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jacques Labuschagne 2004-01-12 04:13:33 UTC
The following function is incorrect:
  template<typename _CharT, typename _Traits, typename _Alloc>
  int
  basic_string <_CharT, _Traits, _Alloc>::
    compare(size_type __pos, size_type __n1, const _CharT* __s,
	    size_type __n2) const

The implementation should only be trying to calculate the length
of __s when __n2 is npos, because __s may have '\0' characters
embedded in it; i.e. it may not be a NTBS.
The use of traits_type::length is still appropriate when __n2
is npos.

Proposed solution:

--- basic_string.tcc.old        2004-01-12 17:12:57.000000000 +1300
+++ basic_string.tcc    2004-01-12 17:13:01.000000000 +1300
@@ -1033,7 +1033,9 @@
       if (__pos > __size)
        __throw_out_of_range("basic_string::compare");

-      size_type __osize = std::min(traits_type::length(__s), __n2);
+      size_type __osize = __n2;
+      if (__n2 == npos)
+          __osize = traits_type::length(__s);
       size_type __rsize = std::min(size_type(__size - __pos), __n1);
       size_type __len = std::min(__rsize, __osize);
       int __r = traits_type::compare(_M_data() + __pos, __s, __len);
Comment 1 Jacques Labuschagne 2004-01-12 04:39:06 UTC
This should also mean that we can unify the two similar
comparisons into one which uses a default 4th argument
(as appears in the standard) -
These two
    compare(size_type __pos, size_type __n1, const _CharT* __s,
	    size_type __n2) const
    compare(size_type __pos, size_type __n1, const _CharT* __s) const
become
    compare(size_type __pos, size_type __n1, const _CharT* __s,
	    size_type __n2 = npos) const
Comment 2 Andrew Pinski 2004-01-12 05:39:51 UTC
From the C++ standard (21.3.6.8):
int compare(size_type pos, size_type n1, charT *s, size_type n2 = npos) const; 

6 Returns: 

	basic_string<charT,traits,Allocator>(*this,pos,n1).compare( 
		basic_string<charT,traits,Allocator>(s,n2)) 


So I agree that they should be combined (they are not on the mainline).
Comment 3 Jacques Labuschagne 2004-01-12 08:01:06 UTC
Created attachment 5459 [details]
Proposed resolution.

diff -u of gcc/libstdc++-v3/include/bits/basic_string.*
Comment 4 Paolo Carlini 2004-01-12 21:04:51 UTC
Hi.
There are a few issues, here, and the proposed patch is incorrect.
1- The standard had a defect, DR 5 (TC) (see also DR 87),

  http://anubis.dkuug.dk/jtc1/sc22/wg21/docs/lwg-defects.html#5

indeed, one of the very first discovered, which implies that we _cannot_
unify the two different compare as you are suggesting.
2- The version missing the fourth parameter is ok as-is.
3- The version with the fourth size_type parameter should be tweaked,
   however, I'm working on it. Basically (see also Josuttis, p. 512),
   '\0' has no special meaning and s _must_ have at least n2 chars:
   therefore __osize == __n2, always.

Thanks,
Paolo.

P.S. I have also checked two other well known implementations and both
are consistent with 1-, 2-, 3- above.
Comment 5 GCC Commits 2004-01-13 11:12:45 UTC
Subject: Bug 13650

CVSROOT:	/cvs/gcc
Module name:	gcc
Changes by:	paolo@gcc.gnu.org	2004-01-13 11:12:38

Modified files:
	libstdc++-v3   : ChangeLog 
	libstdc++-v3/include/bits: basic_string.h basic_string.tcc 
Added files:
	libstdc++-v3/testsuite/21_strings/basic_string/compare/char: 
	                                                             13650.cc 
	libstdc++-v3/testsuite/21_strings/basic_string/compare/wchar_t: 
	                                                                13650.cc 

Log message:
	2004-01-13  Paolo Carlini  <pcarlini@suse.de>
	
	PR libstdc++/13650
	* include/bits/basic_string.tcc (compare(size_type, size_type,
	const _CharT*, size_type)): Implement correctly the resolution
	of DR 5: basically, s is a char array, -not- a C string.
	* include/bits/basic_string.h: Tweak some comments.
	* testsuite/21_strings/basic_string/compare/char/13650.cc: New.
	* testsuite/21_strings/basic_string/compare/wchar_t/13650.cc: New.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/ChangeLog.diff?cvsroot=gcc&r1=1.2214&r2=1.2215
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/include/bits/basic_string.h.diff?cvsroot=gcc&r1=1.40&r2=1.41
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/include/bits/basic_string.tcc.diff?cvsroot=gcc&r1=1.45&r2=1.46
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/testsuite/21_strings/basic_string/compare/char/13650.cc.diff?cvsroot=gcc&r1=NONE&r2=1.1
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/testsuite/21_strings/basic_string/compare/wchar_t/13650.cc.diff?cvsroot=gcc&r1=NONE&r2=1.1

Comment 6 Paolo Carlini 2004-01-13 11:17:40 UTC
Fixed for 3.4.
Comment 7 GCC Commits 2004-01-25 13:13:45 UTC
Subject: Bug 13650

CVSROOT:	/cvs/gcc
Module name:	gcc
Branch: 	gcc-3_3-branch
Changes by:	paolo@gcc.gnu.org	2004-01-25 13:13:42

Modified files:
	libstdc++-v3   : ChangeLog 
	libstdc++-v3/include/bits: basic_string.tcc 

Log message:
	2004-01-25  Paolo Carlini  <pcarlini@suse.de>
	
	PR libstdc++/13650
	* include/bits/basic_string.tcc (compare(size_type, size_type,
	const _CharT*, size_type)): Implement correctly the resolution
	of DR 5: basically, s is a char array, -not- a C string.
	
	* include/bits/basic_string.tcc (_M_clone): Null-terminate.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/ChangeLog.diff?cvsroot=gcc&only_with_tag=gcc-3_3-branch&r1=1.1464.2.166&r2=1.1464.2.167
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/include/bits/basic_string.tcc.diff?cvsroot=gcc&only_with_tag=gcc-3_3-branch&r1=1.29.2.3&r2=1.29.2.4