C++ Difficulty adding an object to an array

Question

My goal is to write a function that adds an object of the AccountInfo class to a 200-element array of AccountInfo objects. The array starts with no objects in it. The AccountInfo class contains several fields - mostly char*, with a few ints.

After hours of attempts, I cannot figure out what is going wrong. My code all complies, but I get an exception

First-chance exception at 0x00A164B0 in Project1.exe: 0xC0000005: Access violation writing location 0x00000000.

on the following line:

getAccounts()[size()] = AccountInfo(*newUser);

I've simplified my code as much as I can while retaining the essential information. If supplying the code for the AccountInfo class would be helpful, I can do that too.

#include <iostream>
using namespace std;

class AccountInfo
{
private:
    char* _userLoginName;   
    char* _password;        
    unsigned int _uid;      
    unsigned int _gid;      
    char* _gecos;           
    char* _home;            
    char* _shell;           

public:
    //Constructor and Destructor
    AccountInfo(char* username);
    ~AccountInfo();

    //Also contains getters and setters.
};

//Method Definitions

AccountInfo::AccountInfo(char* username)
{
    //Initialize the username and other mandatory fields.
    _userLoginName = username;
    _home = "/", "h", "o", "m", "e", "/", username;
    _shell = "/bin/bash";
}

AccountInfo::~AccountInfo()
{
    //Delete dynamically created fields.
    delete _userLoginName;
    delete _password;               
    delete _gecos;          
    delete _home;           
    delete _shell;
}

class UserDB
{
private:
    AccountInfo* _accounts[200];    
    unsigned int _size;             
    unsigned int _nextUid;          
    unsigned int _defaultGid;       

    AccountInfo* getAccounts();

public:
    UserDB();
    ~UserDB();

    void adduser(AccountInfo* newUser); 

    int size(); // return the number of accounts stored (_size)
};

AccountInfo* UserDB::getAccounts()
{
    return _accounts[200];
}

UserDB::UserDB()
{
    _size = 0;
    _nextUid = 1001;
    _defaultGid = 1001;
}

int UserDB::size()
{
return _size;
}

void UserDB::adduser(AccountInfo* newUser)
{
    getAccounts()[size()] = AccountInfo(*newUser);
}

int main() //main method
{
    UserDB users;
    AccountInfo *x = new AccountInfo("Joe"); 
    //This creates an AccountInfo object with one of its
    //char* fields initialized to "Joe".
    users.adduser(x);

    return 0;
}

What am I doing wrong? How can I fix it?

getAccounts()[size()] = AccountInfo(*newUser); did you overload "AccountInfo" to take AccountInfo object? what is expected behavior of that function? — someone_ smiley
– someone_ smiley, Commented Jan 29, 2015 at 2:53
Have you initialized size? Why not just use a std::array or a vector — Karthik T
– Karthik T, Commented Jan 29, 2015 at 2:54
_accounts is an array of pointers, but getAccounts()[size()] = AccountInfo(*newUser); seems to be storing a pointed-to object into the pointer array. How could this ever get compiled? — Lingxi
– Lingxi, Commented Jan 29, 2015 at 2:57

OJFord · Accepted Answer · 2015-01-29 03:29:05Z

2

You're creating a new temporary object to add a pointer to:

getAccounts()[size()] = AccountInfo(*newUser);

Why dereference newUser, only to copy-construct a temporary?

You probably wanted:

getAccounts()[size()] = newUser;

Also, size isn't a great name for that function - it makes it sound as though you're indexing the N+1 position every time. numAccounts or similar is probably more appropriate.

Don't forget to increment this counter, and to check you haven't hit the 200 limit!

Further, with your added code I see that you try to set variables of type char* to "something". You can only do this on initialisation; your constructor needs to set _shell etc. in this manner in an initialisation list, not in the constructor body:

AccountInfo::AccountInfo(char* newUser) : _shell("/bin/bash") {/**/}

edited Jan 29, 2015 at 3:29

answered Jan 29, 2015 at 2:54

OJFord

11.2k10 gold badges71 silver badges101 bronze badges

Sign up to request clarification or add additional context in comments.

1 Comment

user1541090 Over a year ago

Added additional code, including the size() method, getAccounts() method, UserDB constructor, and AccountInfo class definition.

Sam Varshavchik · Accepted Answer · 2015-01-29 02:55:57Z

1

It is not possible to answer your question authoritatively, because you did not show the contents of the size() and getAccounts() methods.

But a good guess can be made:

void UserDB::adduser(AccountInfo* newUser)
{
    getAccounts()[size()] = AccountInfo(*newUser);
}

For this code to work, the getAccounts() method must return a pointer to an existing array of initialized AccountInfo objects, and this code will replace the existing instance of an AccountInfo object, in the array, with a newly-constructed object.

This doesn't really make a lot of sense.

Additionally:

AccountInfo* _accounts[200];

This declares a 200-element array of pointers to AccountInfo objects. This does not declare an array of 200 AccountInfo objects.

I suspect that your getAccounts() method somehow returns the _accounts class member. If so, this cannot possibly ever work this way. Not, at least, unless your constructor dynamically allocates an actual array on the heap, initializes all these pointers to point to their corresponding elements in the dynamically-allocated array, and then always returns the address of the zero-th class instance.

Is that what it does?

answered Jan 29, 2015 at 2:55

Sam Varshavchik

119k6 gold badges109 silver badges164 bronze badges

5 Comments

user1541090 Over a year ago

Added additional code, including the size() method, getAccounts() method, UserDB constructor, and AccountInfo class definition.

Sam Varshavchik Over a year ago

You are constructing an array of 200 pointers, _accounts[0] through _accounts[199], which do not get initialized, and then you return _accounts[200], which does not exist, an uninitialized pointer to random memory, then you try to write to it.

Sam Varshavchik Over a year ago

"return _accounts[200];" does not return the _accounts array to the caller. It returns element #200 from the _accounts array, an array that has only elements #0 through #199. Arrays, in C and C++, don't work the way you think they work.

Sam Varshavchik Over a year ago

Probably your class should declare "AccountInfo _accounts[200];" instead of "AccountInfo* _accounts[200];", and getAccounts() should return &_accounts[0]. This will probably work better, although that really depends on the AccountInfo class. I really hate to provide canned answers like that, because this will not really help you to understand how arrays and pointers work in C and C++.

user1541090 Over a year ago

Yeah, I'm really new to C++ and it's apparent that my understanding of pointers and arrays is flawed. I understand my error in trying to return _accounts[200] though. I appreciate the advice, and I'll keep working at it.

Hien Ngo · Accepted Answer · 2015-01-29 02:54:01Z

0

AccountInfo* _accounts[200];

Because _accounts[index of element] not create using NEW, so _accounts[index of element] is NULL. Please check NULL getAccounts()[size()] before getAccounts()[size()] = AccountInfo(*newUser);

answered Jan 29, 2015 at 2:54

Hien Ngo

2735 silver badges19 bronze badges

1 Comment

user1541090 Over a year ago

Ah, so you're suggesting that I need to initialize the array? I'll try that.

Community · Accepted Answer · 2017-05-23 10:26:28Z

AccountInfo* UserDB::getAccounts()
{
    // This returns a pointer to the element as index 200,
    // which is past the end of the array and 'undefined behavior'
    return _accounts[200];
}

Since you've declared an array of 200 AccountInfo pointers (i.e., not an array of 200 AccountInfo objects) you should return a pointer to the member.

AccountInfo** UserDB::getAccounts()
{
    return _accounts;
}

Furthermore, assuming the addUser function is intended to take ownership of the given pointer it should be updated as follows:

void UserDB::adduser(AccountInfo* newUser)
{
    // This function also needs to guard against size() returning
    // values larger than 199 and increment the size

    // This takes ownership of the pointer and the destructor needs
    // to delete the memory (see 'rule of three' below)
    getAccounts()[size()] = newUser;
}

Also since this is C++ you should be using standard containers or smart pointers to handle this memory management. For example:

std::vector
std::array C++11
std::unique_ptr C++11

Furthermore, if the UserDB class is in fact meant to own the AccountInfo pointers then you should know about the rule of three.

If C++11 is available then you could also implement move semantics.

sv90 · Accepted Answer · 2017-06-11 13:40:40Z

0

Even though this question is two years old I would like to mention an answer that nobody had suggested yet. In my opinion you should use boost::static_vector (http://www.boost.org/doc/libs/1_64_0/doc/html/container/non_standard_containers.html#container.non_standard_containers.static_vector) if it is possible. It has the same behavior that your code tries to have. It constructs an static array with a fixed number of elements and offers the same interface as std::vector. If you try to push more elements than possible it throws an out_of_memory exception just as std::vector.

answered Jun 11, 2017 at 13:40

sv90

5276 silver badges12 bronze badges

Collectives™ on Stack Overflow

C++ Difficulty adding an object to an array

5 Answers 5

1 Comment

5 Comments

1 Comment

Comments

Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

5 Answers 5

1 Comment

5 Comments

1 Comment

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related