User account creation filtered due to spam.

Bug 14245 - [3.4 only] problem with user-defined allocators in std::basic_string
Summary: [3.4 only] problem with user-defined allocators in std::basic_string
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: libstdc++ (show other bugs)
Version: 3.3.3
: P2 normal
Target Milestone: 3.4.1
Assignee: Paolo Carlini
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2004-02-22 19:49 UTC by Chavdar Botev
Modified: 2004-04-21 10:12 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2004-02-25 09:48:57


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Chavdar Botev 2004-02-22 19:49:49 UTC
GCC/libstdc++ 3.3.3-0pre3
Debian Linux Testing("Sarge")

It looks like the STL implementation of libstdc++ 3.3.3.0pre3 assumes that the 
user-defined allocators (at least for basic_string) should be initialized using
the default constructor. 

In particular, the copy constructor for the string representation (struct _Rep)
for basic_string uses the default constructor of the
allocator(basic_string.tcc:192). I think this is wrong: the allocator may need
additional info for proper initialization. It is also unnecessary. Simply using
the copy constructor should be enough.

I think the new line should look something like:

   : _M_dataplus(__str._M_rep()->_M_grab(_Alloc(__str.get_allocator()),
__str.get_allocator()),


Cheers,
Chavdar
Comment 1 Paolo Carlini 2004-02-25 09:48:54 UTC
Hi. Honestly, I'm finding your report a little misleading: there is nothing wrong
with using the default constructor: its availability is among the requirements
for allocators (20.1.5, Table 32), and you find it everywhere in basic_string.

Anyway... probably I get your point: you mean that, at string construction time,
we should use _M_refcopy (not _M_clone) irrespective of the allocator used for
__str, in other terms, _Alloc() is not special.

Do you agree with my exegesis? ;)
Comment 2 Chavdar Botev 2004-02-25 14:01:43 UTC
No. I tend to disagree.

My point is that the copy constructor of basic_string should provide "the most
possible information" to the constructor of the allocator. In the case of the
copy constructor of basic_string, this means invoking the copy constructor of
the allocator and not the default constructor. I don't have the C++ standard
handy:  does it require a copy constructor for the allocators?

Let me give you an example. The project I work on uses memory pools. We have
allocators that provide an interface for the STL objects to a memory pool. To be
properly initialized, these allocators need to get a pointer to a memory pool.
Therefore, we cannot possibly initialize the allocator using the default
constructor. On the other hand, if the copy constructor of the allocator is
invoked, it can use the memory pool associated with the other allocator.

I am also not sure what do you mean by using _M_refcopy instead of _M_clone. Do
you mean changing _M_grab()? If yes, this is not correct. _M_grab() is also used
by the assignment operator and the latter needs the _M_clone() functionality. If
you mean using _M_refcopy() in the copy constructor of basic_string (instead of
_M_grab()), I also think this may be incorrect. You don't know what the
constructor of the allocator does (whether the default or the copy constructor).
For example, in the case of memory pools, it may decide to create/use another
memory pool. In this case, the string should be cloned and not copied.

In any case, if the C++ standard requires the implementation of a copy
constructor for allocators, then I think it should be used in the copy
constructor of basic_string. Then, the implementation of the == operator for
allocators will determine, whether _M_refcopy() or _M_clone() will be used in
_M_grab(). If the copy constructor for allocators is not required, I guess I
have to live with my current workaround: subclassing basic_string and
implementing the copy constructor using the assignment operator instead of the
copy constructor of basic_string. Presumably, it is less efficient but it does
the job.

Cheers,
Chavdar
Comment 3 Paolo Carlini 2004-02-25 14:23:41 UTC
Ok. Now I see what you mean: 21.3.5: "... In the first form the Allocator value
used is copied from str.get_allocator()"

Have you got a testcase?
Comment 4 Paolo Carlini 2004-02-25 15:43:15 UTC
... of course I really meant 21.3.1, p5.

Anyway, via the second argument passed to _M_dataplus we are *already* using
the copy constructor to initialize the underlying _Alloc.

What should be improved (notice, *improved* as a QoI issue, the current
behavior is standard conforming) is what we do for the first argument:
_M_grab (basing on _Alloc(), and __str.get_allocator()) either calls
_M_refcopy or _M_clone(__alloc1), that is _M_clone(_Alloc()), that is does
*not* use the allocator of the new string!

In my opinion, we should first copy construct the allocator from __str, then
either _M_refcopy or _M_clone (therefore allocate memory) via the allocator
itself, the one of the string being constructed.

Agreed?
Comment 5 Paolo Carlini 2004-02-25 15:53:01 UTC
> What should be improved (notice, *improved* as a QoI issue, the current
> behavior is standard conforming) is what we do for the first argument:
> _M_grab (basing on _Alloc(), and __str.get_allocator()) either calls
> _M_refcopy or _M_clone(__alloc1), that is _M_clone(_Alloc()), that is does
> *not* use the allocator of the new string!

Actually *this* behavior seems wrong, not just a QoI issue. Bad things can
happen at deallocation time, when the real allocator of the string will be
used...

Comment 6 CVS Commits 2004-03-28 16:27:38 UTC
Subject: Bug 14245

CVSROOT:	/cvs/gcc
Module name:	gcc
Changes by:	paolo@gcc.gnu.org	2004-03-28 16:27:27

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

Log message:
	2004-03-28  Chavdar Botev  <cbotev@yahoo.com>
	
	PR libstdc++/14245
	* include/bits/basic_string.tcc
	(basic_string::basic_string(const basic_string&)): Pass to
	_Rep::_M_grab the actual allocator of the string being constructed
	not the default constructed one.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/ChangeLog.diff?cvsroot=gcc&r1=1.2429&r2=1.2430
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/include/bits/basic_string.tcc.diff?cvsroot=gcc&r1=1.67&r2=1.68

Comment 7 CVS Commits 2004-04-21 10:11:15 UTC
Subject: Bug 14245

CVSROOT:	/cvs/gcc
Module name:	gcc
Branch: 	gcc-3_4-branch
Changes by:	paolo@gcc.gnu.org	2004-04-21 10:11:10

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

Log message:
	2004-04-21  Chavdar Botev  <cbotev@yahoo.com>
	
	PR libstdc++/14245
	* include/bits/basic_string.tcc
	(basic_string::basic_string(const basic_string&)): Pass to
	_Rep::_M_grab the actual allocator of the string being constructed
	not the default constructed one.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/ChangeLog.diff?cvsroot=gcc&only_with_tag=gcc-3_4-branch&r1=1.2224.2.78&r2=1.2224.2.79
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/include/bits/basic_string.tcc.diff?cvsroot=gcc&only_with_tag=gcc-3_4-branch&r1=1.46.4.4&r2=1.46.4.5

Comment 8 Paolo Carlini 2004-04-21 10:12:09 UTC
Fixed for 3.4.1.
Comment 9 CVS Commits 2004-06-07 22:24:45 UTC
Subject: Bug 14245

CVSROOT:	/cvs/gcc
Module name:	gcc
Branch: 	hammer-3_3-branch
Changes by:	paolo@gcc.gnu.org	2004-06-07 22:24:40

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

Log message:
	2004-06-08  Chavdar Botev  <cbotev@yahoo.com>
	
	PR libstdc++/14245
	* include/bits/basic_string.tcc
	(basic_string::basic_string(const basic_string&)): Pass to
	_Rep::_M_grab the actual allocator of the string being constructed
	not the default constructed one.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/ChangeLog.hammer.diff?cvsroot=gcc&only_with_tag=hammer-3_3-branch&r1=1.1.2.16&r2=1.1.2.17
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/include/bits/basic_string.tcc.diff?cvsroot=gcc&only_with_tag=hammer-3_3-branch&r1=1.28.2.7&r2=1.28.2.8