C++

Forum

 
Bad alloc exception from copy cons?

Pages: 12
adam2016 (1525)
Hi guys I can't seem to figure out why a bad alloc is being thrown,I am guessing it has something to do with the copy constructor but can't see where,when I try to create a person object using the copy constructor a bad alloc gets thrown


thanks 

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
 
  #include <stdio.h>
#include <iostream>
#include <cstring>

using namespace std;

class person{

     public:

     string name;
     int age;
     char* cstr;
     int len;

     person(string n,int age,const char* c):
      name(n),age(age),cstr(new char[len+1])
     {
         len = strlen(c)+1;
         memcpy(cstr,c,len);
     }
     Person(){}

     person(const person &other){

         name = other.name;
         age = other.age;
         len = other.len;
         cstr = new char[len];
         memcpy(cstr,other.cstr,len);
     }
     person(person &&other){

         name = other.name;
         age = other.age;
         len = other.len;
         cstr = other.cstr;
         other.cstr = nullptr;
     }

     person& operator=(person other){

        swap(*this,other);
        return *this;

     }

     void swap(person& one,person &two){

        using std::swap;
        swap(one,two);
     }
};

int main()
{
  person a("adam",24,"hello");
  person b = a;

}
Last edited on
Ganado (6784)
First, your class is person, not Person. Your line 22 is not a valid function. Just be careful of that, is all.

1
2
3
4
5
 
    int len;
     person(string n,int age,const char* c):
      name(n),age(age),cstr(new char[len+1])
     {
         len = strlen(c)+1;

What is the value of len on your line 17 (line 3 in my snippet)?

Note: You are not implementing a destructor. Remember the rule of 5 in C++.
http://en.cppreference.com/w/cpp/language/rule_of_three
(previously called the rule of 3)

1
2
3
4
5
 
     void swap(person& one,person &two){

        using std::swap;
        swap(one,two);
     }


This is infinite recursion. If you want to define assignment in terms of swapping, you have to swap each component separately, or else you'd just be recursively defining the assignment operator with the assignment operator.

https://stackoverflow.com/questions/4782692/what-does-using-stdswap-inside-the-body-of-a-class-method-implementation-mea
Last edited on
adam2016 (1525)
good point Ganado

I tried changing it I added a constructor and defined len before creating the c style string

but still getting a bad alloc 


1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
 

#include <stdio.h>
#include <iostream>
#include <cstring>

using namespace std;

class person{

     public:

     string name;
     int age;
     char* cstr;
     int len;

     person(string n,int age,const char* c):
      name(n),age(age),len(strlen(c)+1),cstr(new char[len])
     {
         //len = strlen(c)+1;
         memcpy(cstr,c,len);
     }
     person(){

     }

     person(const person &other){

         name = other.name;
         age = other.age;
         len = other.len;
         cstr = new char[len];
         memcpy(cstr,other.cstr,len);
     }
     person(person &&other){

         name = other.name;
         age = other.age;
         len = other.len;
         cstr = other.cstr;
         other.cstr = nullptr;
     }

     person& operator=(person other){

        swap(*this,other);
        return *this;

     }

     void swap(person& one,person &two){

        using std::swap;
        swap(one,two);
     }

     ~person(){

        delete[] cstr;
     }
};

int main()
{
  person a("adam",26,"hello");
  cout << a.len << endl;
  person b = a;

}


thanks
jonnin (11333)
it won't compile: line 22.
that aside, why use memcpy instead of strcpy for c-string?
why use c-string and std::string both? These are not errors, but your design makes no sense to me. Why wrap std swap in an extra layer? And your class variables are public, which is frowned upon.

after fixing line 22, it worked fine for me, no bad-allocs.
Are you sure you actually recompiled, given this error, or are you possibly running an older broken executable?


Ganado (6784)
Your swap function is still infinite recursion. I edited my previous post to explain it a bit.

But for your pertinent issue:

Always make sure you're compiling with all warnings enabled. (-Wall in GCC)
1
2
3
4
5
 
 In constructor 'person::person(std::string, int, const char*)':
15:10: warning: 'person::len' will be initialized after [-Wreorder]
14:12: warning:   'char* person::cstr' [-Wreorder]
17:6: warning:   when initialized here [-Wreorder]
 


1
2
3
4
5
6
7
8
9
 
     char* cstr;
     int len;

     person(string n,int age,const char* c):
      name(n),age(age),len(strlen(c)+1),cstr(new char[len])
     {
         //len = strlen(c)+1;
         memcpy(cstr,c,len);
     }


The warning is saying that len will be initialized after cstr.
You want len to be initialized before cstr, because cstr is dependent on len.

The ordering in your initializer list should match the ordering that your class member variables are declared in your class definition.

1
2
3
4
5
6
7
8
9
10
11
 
     string name;
     int age;
     int len;
     char* cstr;

     person(string n,int age,const char* c):
      name(n),age(age),len(strlen(c)+1),cstr(new char[len])
     {
         //len = strlen(c)+1;
         memcpy(cstr,c,len);
     }
Last edited on
adam2016 (1525)
thanks guys,

and yes after changing the order of len and cstr it compiled fine no exceptions but I'm quite surprised at this,I never knew the order would matter since,

now I thought the order would matter in the constructor because yes cstr is dependent on len,but I set the value of length before I initialised cstr to point to a new char array right?

wouldn't length be initialised in the initialiser list before cstr?

I didn't think the order would have mattered when just declaring the variables


and I never knew that :o I thought by enabling ADL by saying using std::swap this would prevent infinite recursion by calling std::swap and not swap

Last edited on
jonnin (11333)
that is really odd, and a good catch. I had both wall and wextra and got no complaint, and as I said, it worked on mine with no problem. Not sure how that happened.
adam2016 (1525)
also for example if I did it like this(didn't use an initaliser list)

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
 


     string name;
     int age;
     char* cstr;
     int len;

     person(string n,int a,const char* c)
     {
         name = n;
         len = strlen(c)+1;
         age = a;
         cstr = new char[len];
         memcpy(cstr,c,len);
     }


I wouldn't get a bad alloc exception,but how come even when len is initialised before cstr in the initaliser list it throws a bad alloc
Ganado (6784)
I really hate it when this forum does the infinite "Sending..."

@ initializer list stuff

I agree it's unintuitive behavior, but here's what the standard says


12.6.2.5

Initialization shall proceed in the following order:

...

Then, nonstatic data members shall be initialized in the order they were declared in the class definition (again regardless of the order of the mem-initializers).
...



This means that if you declare variables as 
1
2
3
 
int a;
int b;
int c;


But then to initialize in the order a, c, b, it doesn't matter that you try to initialize c before b, it will always attempt to initialize b before c.
Whether you want to or not, it is doing 
1
2
3
 
    person(string n,int age,const char* c):
      name(n),age(age),cstr(new char[len]), len(strlen(c)+1)


tl;dr Make sure variable declaration matches initialization order. (Also things like visibility (public/private) does not affect this, the only thing that matters it's sequential ordering).


@ std::swap
I thought by enabling ADL by saying using std::swap this would prevent infinite recursion by calling std::swap and not swap

Let's suppose it did call std::swap(T& a, T& b) where T = person.

The code would look like this
person temp = a;
a = b;
b = temp;

But... how does a person get assigned to another person...? Well, through the assignment operator... which is defined through swap.
Infinite recursion; swap each component instead.
Last edited on
Ganado (6784)
Just to be clear: The bad alloc most likely happens because you are using len before it is initialized, and you're attempting to do new char[len].
Since len is uninitialized, it's undefined behavior, but most likely len will have a junk value in it like -1458329, and the bad alloc is from trying to allocate a negative size (or a ridiculously large size).
Thomas1965 (4571)
If you declare len before cstr it works.
I guess it's the problem with the order of initialization.
In the original code len had some negative value.

BTW. In this case the order of declaration is not important
1
2
3
4
5
6
7
8
9
 
  person(string n, int age, const char* c)
  {
    name = n; 
    age = age; 
    assert(c != nullptr);
    len = strlen(c) + 1; 
    cstr = new char[len];
    memcpy(cstr, c, len);
  }


BTW: Why don't you learn how to use a debugger ?
Last edited on
adam2016 (1525)
thanks guys amazing answer Ganado :)

to be honest that is pretty strange behaviour I don't know why they would implement the initaliser list like that there must be a reason why

and agreed that's why I always copy my post before I send it because sometimes it just doesn't send.

@Thomas very true,I have been programming for 2 years now properly I think it's well overdue that I learn how to use a debugger.

thanks
adam2016 (1525)
@Ganado have just one more question sorry about so many questions,

Let's suppose it did call std::swap(T& a, T& b) where T = person.

The code would look like this
person temp = a;
a = b;
b = temp;

But... how does a person get assigned to another person...? Well, through the assignment operator... which is defined through swap.
Infinite recursion; swap each component instead.



I still kind of fail to see how an infinite recursion would take place,temp would = a,then a would = b,then b would = temp,but how would that cause infinite recursion all them statements are logically correct?

the assignment operator is defined through swap? I thought the assignment operator would be defined in the person class?

thanks
Ganado (6784)
First, let me clarify that it is not calling std::swap in your case, I was being hypothetical.

In your case, your swap function is calling itself directly. A non-template definition will be preferred to a template definition when the compiler is trying to figure out which function to call.

For example,
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
 
#include <iostream>

// template defined first
template <typename T>
void foo(T& t)
{
    std::cout << "template T& foo" << std::endl;   
}

void foo(int& t)
{
    std::cout << "int& foo" << std::endl;   
}

// int& defined first
void bar(int& t)
{
    std::cout << "int& bar" << std::endl;   
}

template <typename T>
void bar(T& t)
{
    std::cout << "template T& bar" << std::endl;   
}

int main()
{
    int t = 0;
    foo(t); // calls int& version, not template version
    bar(t);  // calls int& version, not template version
}


___________________________________________

But besides that, let me focus on what you said:

I still kind of fail to see how an infinite recursion would take place,temp would = a,then a would = b,then b would = temp,but how would that cause infinite recursion all them statements are logically correct?


b = temp;
Let's focus on just this:
b is a person.
temp is a person.

How is temp, a person, being assigned to b, a person?


Through the operator= for person. If it's not using the operator=, what operator do you think it's using?

the assignment operator is defined through swap? I thought the assignment operator would be defined in the person class?

Those are not conflicting statements.
Your assignment operator is defined through swap.
And your assignment operator is defined in your person class.

Both are true, correct.

Your code:
1
2
3
4
5
 
     person& operator=(person other){

        swap(*this,other);
        return *this;
     }

As you have it now, your assignment operator is dependent on a function called swap. You're not "using namespace std", so there's only 1 option for the compiler to choose. The function you defined that takes in two person references.

Your function,
1
2
3
4
 
     void swap(person& one,person &two){
        using std::swap;
        swap(one,two);
     }

matches this call, so the compiler will try to call the function you defined, from your operator= function.

Inside this function, there is a bit of ambiguity for whether or not it calls std::swap vs. your custom swap, but because of the way that templates are deduced, it will prefer calling the existing swap(person&, person&) function, i.e. itself, recursively.


Hope that makes sense, if not I can make an example that's simpler than your OP post.

__________________________________________________________

using std::swap; is usually done in a templated function, because you're not sure what type you're actually attempting to use swap on; a built-in type that can just use the standard std::swap, or a custom type that would have its own free swap function associated with it.

_____________________________________________________________

1
2
3
4
5
6
 
     void swap(person& one, person &two){
        std::swap(one.name, two.name);
        std::swap(one.age, two.age);
        std::swap(one.len, two.len);
        std::swap(one.cstr, two.cstr);
     }


or

1
2
3
4
5
6
 
     void person::swap(person &other){
        std::swap(name, two.name);
        std::swap(age, two.age);
        std::swap(len, two.len);
        std::swap(cstr, two.cstr);
     }


I've made this more confusing than necessary, methinks.
Last edited on
adam2016 (1525)
thanks Ganado :)

no awesome answer I actually understand :) one of the trickiest things I've come across I may be one in a few but I prefer C++ prior to C++11 so many changes

just a quick follow up and I hope I didn't miss anything sorry if I did,so why should we swap a persons member variables one for one with another persons members instead of just swapping the person object itself?

This is infinite recursion. If you want to define assignment in terms of swapping, you have to swap each component separately, or else you'd just be recursively defining the assignment operator with the assignment operator.


could std::swap() not jut swap full objects?
Last edited on
icy1 (596)
As per http://en.cppreference.com/w/cpp/algorithm/swap , the Person would need to be both move assignable and move constructible

I think this showcases most of it... running at https://repl.it/@icy_1/UpbeatValuableDatawarehouse
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
 
#include <iostream>

using namespace std;

// Default: both commented out.  
// If either one uncommented, cant do a std::swap
//#define FORCE_NOT_MOVE_CONSTRUCTIBLE 1
//#define FORCE_NOT_MOVE_ASSIGNABLE 1

class Person
{

public:
    Person(int age) : age_(age)
    {        
    }
    Person(const Person &other)
    {
        cout << "copy constructor\n";
        age_ = other.age_;
    }  

    Person& operator=(const Person& other)
    {
        age_ = other.age_;
        std::cout << "copy assigned\n";
        return *this;
    }
#ifdef FORCE_NOT_MOVE_CONSTRUCTIBLE
protected:
#endif
    Person(Person&& other) : age_(move(other.age_))
    { 
        cout << "move constructor\n";
    }
#ifdef FORCE_NOT_MOVE_CONSTRUCTIBLE
public:
#endif

#ifdef FORCE_NOT_MOVE_ASSIGNABLE
protected:
#endif
    Person& operator=(Person&& other)
    {
        age_ = move(other.age_);
        cout << "move assigned\n";
        return *this;
    }
#ifdef FORCE_NOT_MOVE_ASSIGNABLE
public:
#endif

    int Age() { return age_; }

private:
    int age_;
};

int main()
{
    cout << boolalpha;
    
    cout << "move assignable? " << is_move_assignable<Person>::value << endl;
    cout << "move constructible? " << is_move_constructible<Person>::value << endl;
    cout << endl << endl;

    cout << "Person a(15); Person b = a;  ";
    Person a(15);
    Person b = a;
    cout << "Person b2(a);  ";
    Person b2(a);  // copy constructor

    cout << "Person c(12); c = a;  ";
    Person c(12);
    c = a;  // copy assignment
    

#if !defined(FORCE_NOT_MOVE_ASSIGNABLE) && !defined(FORCE_NOT_MOVE_CONSTRUCTIBLE)
    cout << endl << "swap(a,b):"<< endl;
    swap(a,b);
    cout << endl << endl;
#endif

    cout << "Person d(29);\n";
    Person d(29);

#ifndef FORCE_NOT_MOVE_CONSTRUCTIBLE
    cout << "Person m = move(d);  ";
    Person m = move(d); // move constructor
    cout << "Person n(d);  ";
    Person n(move(d));  // move constructor
#endif

#ifndef FORCE_NOT_MOVE_ASSIGNABLE
    cout << "Person o(18); o = move(d);  ";
    Person o(18);
    o = move(d); // move assignment
#endif

    return 0;
}


Edit: cleaned up and rearranged a bit
Last edited on
Ganado (6784)
could std::swap() not jut swap full objects?

But that's exactly the problem -- how does it know to swap full objects? The non-specialized version of the std::swap template uses the operator= within its logic. So from swap(), you'd be calling the assignment operator, which calls swap, which calls the assignment operator, which calls swap, which calls the assignment operator, which calls swap, which calls the assignment operator... etc.

https://stackoverflow.com/questions/25286544/how-does-the-standard-library-implement-stdswap

Some interesting links:
http://en.cppreference.com/w/cpp/algorithm/swap
std::swap may be specialized in namespace std for user-defined types, but such specializations are not found by ADL (the namespace std is not the associated namespace for the user-defined type). The expected way to make a user-defined type swappable is to provide a non-member function swap in the same namespace as the type: see Swappable for details.


Shows an example of implementing your own swap:
http://en.cppreference.com/w/cpp/concept/Swappable
Last edited on
adam2016 (1525)
thanks guys really interesting that is a lot more complicated then I thought it would be :O

I enabled the c++11 flag on my compiler and the code compiles fine and actually gives the correct results b and c both print "adam" out and the program terminates a it should,

but no infinite recursion?

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
 


#include <stdio.h>
#include <iostream>
#include <cstring>

using namespace std;

class person{

     public:

     string name;
     int age;
     char* cstr;
     int len;

     person(string n,int a,const char* c)
     {
         name = n;
         len = strlen(c)+1;
         age = a;
         cstr = new char[len];
         memcpy(cstr,c,len);
     }
     person(){

     }

     person(const person &other){

         name = other.name;
         age = other.age;
         len = other.len;
         cstr = new char[len];
         memcpy(cstr,other.cstr,len);
     }
     person(person &&other){

         name = other.name;
         age = other.age;
         len = other.len;
         cstr = other.cstr;
         other.cstr = nullptr;
     }

     person& operator=(person other){

        swap(*this,other);
        return *this;

     }

     void swap(person& one,person &two){

        using std::swap;
        swap(one,two);
     }

     ~person(){

        delete[] cstr;
     }
};

int main()
{
  person a("adam",26,"hello");
  cout << a.len << endl;
  person b = a;
  cout << b.name << endl;
  person c = a;
  cout << c.name << endl;
}
adam2016 (1525)
But that's exactly the problem -- how does it know to swap full objects? The non-specialized version of the std::swap template uses the operator= within its logic. So from swap(), you'd be calling the assignment operator, which calls swap, which calls the assignment operator, which calls swap, which calls the assignment operator, which calls swap, which calls the assignment operator... etc.


yes that makes sense now! :) but lets say an object had another object as a member variable(which also called swap() in its operator= function) as in composition,that means we couldn't use the std::swap() function? because std::swap would call the assignment operator on that object right?

also the strange thing is the above code compiles and executes fine no sign of infinite recursion,a you said there should be infinite recursion because the = operator will call swap and swap calls the = operator etc

thanks
Ganado (6784)
True, I wasn't being specific enough when talking about when recursion happens.

Your operator= is actually not being called in your particular code, because the compiler optimizes the
Type b = a; initialization assignment to just be a copy constructor.

But if you did
1
2
3
 
Type a;
Type b;
b = a;


Line 3 will call your assignment operator, unless your compiler does some drastic optimization or something.

Try this out, a modification of your above post:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
 
     // ... 

     void swap(person& one,person &two){

        std::cout << "help!" << std::endl;
        using std::swap;
        swap(one,two);
     }

     ~person(){

        delete[] cstr;
     }
};

int main()
{
  person a("adam",26,"hello");
  cout << a.len << endl;
  person b;
  b = a; // actually calls the assignment operator!   RECURSION !
  cout << b.name << endl;

}


Once you practice correctly implementing the assignment operator, you should also implement the move assignment operator (see icy1's post for a simpler example).
Last edited on
Pages: 12