Sign in to follow this  
trick

problem using vector

Recommended Posts

I recently tried to alter some code from using arrays to using vectors. Everything I've read seemed like it would be easy enough, but I've run into a few problems.
#include <vector>

vector<int> v1;
vector v2;

Given the "source" above, I have two different problems. (And yes, I know that the vector v2 is incorrect, but it shows some info that seems peculiar). When I use the v1 version, I cannot access any functions of the vector. Using VC++6, I don't even get the list of functions available when typing "v1." that usually pops up automatically. If I leave out the type, the list pops up as normal, but with the obvious problem of not having any type set. Can anyone see what I might be doing wrong? If needed, I will supply full source code...

Share this post


Link to post
Share on other sites
Are you doing:


using namespace std;

or

using std::vector;

?

If yes then paste the full code. Also, the intellisense for VS6.0 was pretty wobbley at times, a better test of correct syntax is whether it compiles.

Dave

Share this post


Link to post
Share on other sites
No, I'm not. although the above snippet isn't copied from my source code, it's exactly what my source code has in it. (minus the v2 line).

I did try to create a new project, a simple "Hello World" console app, and included vector there in the same way I am in my current project, and it works fine there.

sorry, slight edit. Just noticed something else...

//When I type it like this....

vector<int> v1;
v1.push_back(4);
//it works fine while designing, but won't compile. when I do this...

std::vector<int> v1;
v1.push_back(4);
//I get the above problems, but the program compiles fine.

Share this post


Link to post
Share on other sites
Quote:
Original post by Dave
If yes then paste the full code. Also, the intellisense for VS6.0 was pretty wobbley at times, a better test of correct syntax is whether it compiles.


Yeah, I was just going to say this.

Does your code compile? If it compiles it's just a problem with VS6.0 (which is ungodly horrible). There's not really anything you can do about that, it's an old piece of crap IDE. you should upgrade to Visual Studio 2005 (you can download the Express edition for free from MS).

-me

Share this post


Link to post
Share on other sites
I should have mentioned that doing either of the 2 lines i coded in my previous post means that you don't need to put std:: on the front of vector. However not doing either of the lines i mentioned means that the second piece of code in your last post is the valid way of doing it, the first is wrong.

Dave

Share this post


Link to post
Share on other sites
you know you don't have to use intellisense. I never use it. I like typing out the whole function name. As long as your code compiles and works does it really matter what intellisense does?

Share this post


Link to post
Share on other sites
Quote:
Original post by trick
Using VC++6, ...

That's your problem.

Seriously, that compiler is obsolete. It has problems with templates and has issues with the C++ standard. There is no reason for anyone to be using it. I recommend that you switch to any current compiler. If you like Visual Studio, then try Visual Studio 2005 Express Edition.

Share this post


Link to post
Share on other sites
Looking into upgrading to Express, just trying to go through the downloads for .net framework, or whatever it needs....

full source

#include "stdafx.h"
#include <vector>

int main(int argc, char* argv[])
{
std::vector<std::string> v1;
v1.push_back("Hello World");
printf(v1[0].c_str());
return 0;
}

;



I'm not so much worried about the intellisence, and the above snippet works (though the intellisence doesn't work). It seems I'm just using vector wrong.

I was trying to use it to store pointers to structures. A seperate function would do...

//vec1 created outside of function, with vector<myStruct*> vec1
myStruct* t1;
t1 = new myStruct;
vec1.push_back(t1);



My understanding was, since the object was created with new, it would have no problem being added and not going out of scope at the end of the function. (Until I manually use delete to remove the item). Is my understanding wrong?

And also, thanks for the help, and quick response!

Share this post


Link to post
Share on other sites
The vector stores pointers to the data here so all the vector clears up here is the pointers, leaving the memory floating. You need to iterate through the vector and delete what each pointer points to and then call erase() on the vector to clean that up.

In your example the pointer t1 will go out of scope and be destroyed, but you will still have the data allocated by new and that won't be cleaned up. You also won't lose the data because a copy of the pointer is made when you added it to the vector. All it fine here.

Dave

Share this post


Link to post
Share on other sites
Snippets from my actual code (not the test program i posted earlier).

void ZTemplateSet3::load(LPDIRECT3DDEVICE9 pd3dDevice, std::string name, std::string ext){
cleanup();
std::string file;
file = name;
name.append(ext);
load_img(pd3dDevice, file, ext);
//continue loading here...
file = name;
file.append(".zt3");
fstream xFile;
xFile.open(file.c_str(), ios::in);
int i, j, k;
float p;
int w, x, y, z;
ZTemplate3* t1;
ZTemplateGroup3* t2;
ZTemplateIcon3* t3;
xFile >> i;
templates.reserve(i);
for(int a = 0; a < i; a++){
t1 = new ZTemplate3;
xFile >> j;
templates[i].groups.reserve(j);
for(int b = 0; b < j; b++){
t2 = new ZTemplateGroup3;
xFile >> k;
templates[i]->groups[j]->icons.reserve(k);
for(int c = 0; c < k; c++){
t3 = new ZTemplateIcon3;
xFile >> w >> x >> y >> z;
xFile >> p;
t3->anim_rate = p;
t3->icon.left = w;
t3->icon.right = w + y;
t3->icon.top = x;
t3->icon.bottom = x + z;
t2->icons.push_back(t3);
}
t1->groups.push_back(t2);
}
templates.push_back(t1);
}
xFile.close();
active = true;
}

int ZTemplateSet3::add_template(ZTemplate3* newtemplate){
templates.push_back(newtemplate);
active = true;
return templates.size();
}
//similar functions for adding groups, and icons

void ZTemplateSet3::cleanup(){
if(!active)
return;
ZTemplate3* t1;
ZTemplateGroup3* t2;
ZTemplateIcon3* t3;
for(int i = templates.size()-1; i >= 0; i++){
for(int j = templates[i]->groups.size()-1; j >= 0; j++){
for(int k = templates[i]->groups[j]->icons.size()-1; k >= 0; k++){
t3 = templates[i]->groups[j]->icons[k];
templates[i]->groups[j]->icons.pop_back();
delete t3;
}
t2 = templates[i]->groups[j];
templates[i]->groups.pop_back();
delete t3;
}
t1 = templates[i];
templates.pop_back();
delete t1;
}
active = false;
}




EDIT: After this, if I say int num = templates.size(), I get something like -872541 (just some big negative number...)

Share this post


Link to post
Share on other sites
Quote:
Original post by trick
Snippets from my actual code (not the test program i posted earlier).
*** Source Snippet Removed ***

EDIT: After this, if I say int num = templates.size(), I get something like -872541 (just some big negative number...)


Your code is flawed in a lot of ways. One obvious way is you do reserve, which only makes it so when you DO add elements no resizing occurs. What you may need is resize instead. Also, even if you did resize you have code like this:
templates.reserve(i);
for(int a = 0; a < i; a++){
t1 = new ZTemplate3;
xFile >> j;
templates[i].groups.reserve(j);


Look at 'i'. It doesn't change does it? And yet you try accessing element i of the vector 'templates' so every loop it uses the same value. You probably meant to use a. Not to mention that i is outside the bounds of the reserved vector elements. That is, you can only access an index between 0 and i-1. You'll have to do a little bit of rewriting of the code to make it work(maybe change all occurrences of reserve to resize, but that's only a band-aid over a knife wound). What I suggest is making your classes know how to load themselves from file. Something like this:

#include <fstream>
#include <string>
#include <iostream>
#include <vector>

class ZTemplateIcon3
{
private:
RECT location;
int anim_rate;
public:
ZTemplateIcon3(istream &input)
{
int w, x, y, z, p;
input >> w >> x >> y >> z >> p;
anim_rate = p;
location.left = w;
location.right = w + y;
location.top = x;
location.bottom = x + z;
}
};

class ZTemplateGroup3
{
public:
std::vector<dataTypeHere> * groups;
ZTemplate(istream &input)
{
groups = new std::vector<dataTypeHere>;
int numberOfIcons;
input >> numberOfIcons;
for(int i = 0; i < numberOfIcons; i++)
{
groups->push_back( newIcon(input) );
}
}
};
// ZTemplate3 and ZTemplateSet go here, changed as well to load files like above.

// usage:
someClass::someMethod(const std::string &filename)
{
std::fstream xFile(filename.c_str(), ios::in);
xFile >> numberOfGroups;
for(int i = 0; i < numberOfGroups; i++)
{
ZTemplateGroup3 t1(xFile);
}
// other stuff
}

// etc



I'd ask to see all of your code to help changing it, but I'm a little busy these days so I wouldn't really have the time. Maybe the code I've written will give you ideas on how to get your program working. Try naming your variables something better as well. Even something simple like naming something numberOfWhatever is an improvement. You called something icons when it really just looked like a set of coordinates(a La a RECT which is what ended up making 'location'). the istream thing just uses the well-designed generic input stream. You can send a file stream(std::fstream or std::ifstream), a console stream(std::cin), or even a string stream(std::stringstream). This isn't really important in your case, but it's good for an object to be agnostic about what's doing the loading.

Share this post


Link to post
Share on other sites
Great summary, nobodynews. I can't believe I haven't rated you up before, but you just got The Nod(TM) now, so congrats. :) (EDIT: Although the OP's usage of reserve() actually is correct, here, since the elements are added with .push_back().)

Anyway, thought I'd roll up my sleeves and do some refactoring...


// We extract reading functions. The usual way to spell their names is as
// "operator>>". This lets us keep on using >> syntax all over the place in
// intuitive ways, as long as we set it up right.
// Also notice that this refactoring gets rid of huge blocks of temporary
// variables. To make these work, we may need to declare them as 'friend's of
// the corresponding classes.
istream& operator>>(istream& is, ZTemplateIcon& z) {
int w, x, y, z;
float p;
is >> w >> x >> y >> z >> p;
z.anim_rate = p;
z.icon.left = w;
z.icon.right = w + y;
z.icon.top = x;
z.icon.bottom = x + z;
return is;
}

istream& operator>>(istream& is, ZTemplateGroup3& z) {
is >> count;
z.icons.reserve(count);
for (int i = 0; i < count; ++i) {
ZTemplateIcon3* ti = new ZTemplateIcon3;
is >> *ti;
z.icons.push_back(ti);
}
return is;
}

istream& operator>>(istream& is, ZTemplate3& z) {
is >> count;
z.groups.reserve(count);
for (int i = 0; i < count; ++i) {
ZTemplateGroup3* g = new ZTemplateGroup3;
is >> *g;
z.groups.push_back(g);
}
return is;
}

// names like "load" always make me suspicious. Could this work be done
// in a constructor instead?
void ZTemplateSet3::load(LPDIRECT3DDEVICE9 pd3dDevice,
const std::string& name,
const std::string& ext) {
cleanup();
// We don't need a temporary variable in order to express a
// concatenation of strings. This is the normal way to do it:
load_img(pd3dDevice, name + ext, ext);
// Although, I'm wondering why load_img takes a name with an extension
// and *also* needs the extension info separately. Why not do the
// concatenation inside that function? Or alternatively, have it
// "extract" the extension from the full name?

//continue loading here...

// Initialize variables when you can. The fstream constructor allows you
// to specify the initially-opened file, and this is the normal way to
// use files. .open() and .close() are for the rare occasions when you
// need a stream object to work with more than one file, and some other
// assorted special cases...
// Oh, and we don't need to specify ios::in explicitly, if we just do:
ifstream file((name + ".zt3").c_str());
// Again, using the technique from before. Yes, we can .c_str() a
// temporary string; its scope is to the end of the statement.
int num_templates;
file >> num_templates;
templates.reserve(num_templates);
for (int a = 0; a < num_templates; ++a) {
ZTemplate3* z = new ZTemplate3;
is >> *z;
templates.push_back(z);
}
// No need to close explicitly.
active = true; // This looks suspicious too...
}

int ZTemplateSet3::add_template(ZTemplate3* newtemplate) {
templates.push_back(newtemplate);
active = true;
return templates.size();
}
// similar functions for adding groups, and icons

// Again suspicious (seems like destructor work).
// Is there a reason why you would ever re-load an already "set up" object? Why?
// But regardless, we can at least handle the cleanup recursively via the
// *other* object destructors...
ZTemplateGroup3::~ZTemplateGroup3() {
// There is no need to remove the pointers from the container, because
// the container itself is about to die, taking all the pointers with
// it. The container cleans up its memory; you clean up yours.
// Also, this frees us to avoid doing things backwards.
for (int i = 0; i < icons.size(); ++i) { delete icons[i]; }
}

ZTemplate3::~ZTemplate3() {
// There is no need to remove the pointers from the container, because
// the container itself is about to die, taking all the pointers with
// it. The container cleans up its memory; you clean up yours.
// Also, this frees us to avoid doing things backwards.
for (int i = 0; i < groups.size(); ++i) { delete groups[i]; }
}

void ZTemplateSet3::cleanup() {
if (!active) { return; }

// Similarly here, except that because this isn't a destructor, we do
// need to remove the pointers from the container. We have a smarter
// way, though:
for (int i = 0; i < templates.size(); ++i) { delete templates[i]; }
templates.clear();
active = false;
}

// But again, we probably need to fix the code so that we're using constructors
// and destructors instead of load() and cleanup() functions. It's called RAII,
// don'cha know. :)




And even at that, things are probably much more complicated than they need to be. In particular, I don't yet see a reason for storing pointers in the vectors instead of storing object instances directly.

Share this post


Link to post
Share on other sites
Seems I have a lot to go through! On the bright side, most of what has been said makes perfect sense (at least now it does), it's just things that I haven't dealt with much yet. But on that note, there's probably something already out there that will let me do everything I need, so this is more of a learning experience anyways.

Thanks for all the help! (Finally got VC++2005 express, so I think I'll update as I transfer).

Share this post


Link to post
Share on other sites
He does push_back, but not until after he accesses 'templates'. Look at the code again and step through it:
templates.reserve(i); // reserve space for i elements
for(int a = 0; a < i; a++){ // loop from 0 to i
t1 = new ZTemplate3; // point t1 to memory allocated by new to a ZTemplate3
xFile >> j; // read in the next number from the file
templates[i].groups.reserve(j); // access the ith element of templates
// to reserve j elements in the member group.


It isn't until the end of the loop that he pushes an element into the vector; by then the damage has been done.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this