Sign in to follow this  
BiiXteR

Why can't I print out my std::string vector?

Recommended Posts

For some reason my application crashes when I try to print out the values of my std::vector, which is filled with std::strings.

 

Here's how I declare the vector : 

	std::vector <std::string> map
	{
		"###############################################################",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"###############################################################",
	};

And here's how I try to print it out : 

	for (int i = 0; i < map.size(); i++)
	{
		printf(map[i].c_str());
	}

The crash message I get is : 

 

Exception thrown at 0x01157216 in ConsoleGame.exe: 0xC0000005: Access violation reading location 0x00000018.

Edited by BiiXteR

Share this post


Link to post
Share on other sites

Weird; that code runs fine for me. (It's missing newline characters though)

 

Forgot to mention that the error leads me to line 512 in the file xstring.

Share this post


Link to post
Share on other sites
Try to put together the smallest program that shows the problem. It is likely that you will figure out what the problem is in the process of producing that program. If not, post it here, so we can reproduce the problem on our own.

Share this post


Link to post
Share on other sites

Access violation reading location 0x00000018.

 
 
Looks like you are calling a member function on a null object.
 
Such as:
 
myThing = null;
myThing->DoSomething();
 
Look on your stack trace, go back out on the stack to the outermost function that you wrote, and look for a null pointer on any object.

For example, maybe a null pointer value for the string you are trying to print.
 
 
 

Forgot to mention that the error leads me to line 512 in the file xstring.

 
You're looking in the wrong spot.  While it is remotely possible that you found a bug in the standard library, consider that the libraries are used by millions of programmers around the globe every day.  The odds that it works correctly for everybody else but is broken for you is astonishingly small.  
 
Always assume the error is in your code, not in the standard library. 
 

  for (int i = 0; i < map.size(); i++)
    {
        printf(map[i].c_str());
    }


Following a hunch, it could be that map[i] doesn't exist, or that it doesn't contain a string (it is empty). Edited by frob

Share this post


Link to post
Share on other sites

Are you by any chance using XCode?
 
Btw one more thing: It's a security risk to do printf( myCStringVariable ); Perform instead printf( "%s", myCStringVariable );
 
Cheers


Could you elaborate on why that's a security risk?

Chances are what he really wanted was `std::puts(myCStringVariable)' anyway.

Share this post


Link to post
Share on other sites

Are you by any chance using XCode?
 
Btw one more thing: It's a security risk to do printf( myCStringVariable ); Perform instead printf( "%s", myCStringVariable );
 
Cheers


Could you elaborate on why that's a security risk?


Suppose myCStringVariable contains "%s".
Then we have a statement here that can be rewritten as printf("%s").
This will try to print out whatever is on the stack where printf would look for its second argument - only there isn't one.

This could be anything - a valid pointer, a null pointer, arbitrary data - even executable machine code. An attacker could use this to crash the program, or look at the stack frame and work out where the current function's return address is, or do all manner of nasty things one can do with printf specifiers that point at garbage.

The %n specifier in the wrong hands could be particularly nasty because it allows printf to modify an argument passed to it... Edited by Oberon_Command

Share this post


Link to post
Share on other sites

 

Access violation reading location 0x00000018.

 
 
Looks like you are calling a member function on a null object.
 
Such as:
 
myThing = null;
myThing->DoSomething();
 
Look on your stack trace, go back out on the stack to the outermost function that you wrote, and look for a null pointer on any object.

For example, maybe a null pointer value for the string you are trying to print.
 
 
 

Forgot to mention that the error leads me to line 512 in the file xstring.

 
You're looking in the wrong spot.  While it is remotely possible that you found a bug in the standard library, consider that the libraries are used by millions of programmers around the globe every day.  The odds that it works correctly for everybody else but is broken for you is astonishingly small.  
 
Always assume the error is in your code, not in the standard library. 
 

  for (int i = 0; i < map.size(); i++)
    {
        printf(map[i].c_str());
    }

Following a hunch, it could be that map[i] doesn't exist, or that it doesn't contain a string (it is empty).

 

 

I checked the size of the vector, which for some reason is 0.

I don't understand why though, I've tried giving it a value in the constructor, and in the header file and it still has a value of 0.

 

Here's some more code : 

 

MapManager.h

#pragma once

#include <vector>
#include <iostream>

class MapManager
{

public:

	std::vector <std::string> map =
	{
		"###############################################################",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"###############################################################"
	};

	void DrawMap();

	static MapManager &GetInstance();

private:

	MapManager();
	~MapManager();

};

MapManager.cpp

#include "MapManager.h"



MapManager::MapManager()
{

}

void MapManager::DrawMap()
{

	std::cout << map[0].c_str() << std::endl;

	for (int i = 0; i < map.size(); i++)
	{
		printf(map[i].c_str());
	}
}

MapManager &MapManager::GetInstance()
{
	MapManager temp;
	return temp;
}

MapManager::~MapManager()
{
}

Edited by BiiXteR

Share this post


Link to post
Share on other sites

 

MapManager &MapManager::GetInstance()
{
  MapManager temp;
  return temp;
}

This is the cause of your problem.  You end up with a reference to a deleted object because the referent goes out of scope after the return statement.  Very bad.

Global variables need to be in static storage. Either make temp a static local, a cass variable, or move it out of your class entirely and make it a namespace-level variable. It's your disguising the global as something else that's hiding your problem in the first place.  Another lesson in why globals are poor practice.

 

 

ohh, I forgot to add a static to the temp variables, how did I not notice that o.o

Thank you though :P

Share this post


Link to post
Share on other sites

ohh, I forgot to add a static to the temp variables, how did I not notice that o.o
Thank you though :P


Seriously, just don't do that.
Let's have a look at your code.

#pragma once

#include <vector>

// don't need iostream for the header
// as a general rule, don't force consumers of a class to include something they don't need
//#include <iostream>

class MapManager
{

// do you really want map to be public? 
// anyone using this class could easily overwrite the contents of it
// public:

	std::vector <std::string> map =
	{
		"###############################################################",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"###############################################################"
	};

// ok, this needs to be public
public: 
	void DrawMap();

// ugh, singleton... no need for it. 
//	static MapManager &GetInstance();

// don't need these, compiler will auto generate them for us
//private:
//	MapManager();
//	~MapManager();

};

cpp

#include "MapManager.h"

// include iostream where it's needed
#include <iostream>
// don't need ctor or dtor

void MapManager::DrawMap()
{
    // as khatharr said, but without the evil braceatendofstatement style :p
    for(auto& line : map)
    {
       std::cout << line << "\n";
    }
}

// nope nope nope nope nope
//MapManager &MapManager::GetInstance()
//{
//	MapManager temp;
//	return temp;
//}

and using it

MapManager map;
map.DrawMap();

so much cleaner... less code, less bugs... easy to use, hard to abuse.

Edited by ChaosEngine

Share this post


Link to post
Share on other sites

As a side note, you should not pass your strings as the format argument to printf(). Either do this:

printf("%s", map[i].c_str());

Or use puts(), since you're not really formatting anything (note: adds a newline for you)

puts(map[i].c_str());

The reason for this is because otherwise it's possible to pass arbitrary format options to printf() without meaning to (say your map contained "%" characters) and it can possibly be dangerous.

Edited by TheComet

Share this post


Link to post
Share on other sites

As a side note, you should not pass your strings as the format argument to printf(). Either do this:

printf("%s", map[i].c_str());
Or use puts(), since you're not really formatting anything (note: adds a newline for you)
puts(map[i].c_str());
The reason for this is because otherwise it's possible to pass arbitrary format options to printf() without meaning to (say your map contained "%" characters) and it can possibly be dangerous.


We've already discussed that in this thread: http://www.gamedev.net/topic/681722-why-cant-i-print-out-my-stdstring-vector/#entry5308405

Share this post


Link to post
Share on other sites

 

ohh, I forgot to add a static to the temp variables, how did I not notice that o.o
Thank you though :P


Seriously, just don't do that.
Let's have a look at your code.

#pragma once

#include <vector>

// don't need iostream for the header
// as a general rule, don't force consumers of a class to include something they don't need
//#include <iostream>

class MapManager
{

// do you really want map to be public? 
// anyone using this class could easily overwrite the contents of it
// public:

	std::vector <std::string> map =
	{
		"###############################################################",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"#                                                             #",
		"###############################################################"
	};

// ok, this needs to be public
public: 
	void DrawMap();

// ugh, singleton... no need for it. 
//	static MapManager &GetInstance();

// don't need these, compiler will auto generate them for us
//private:
//	MapManager();
//	~MapManager();

};

cpp

#include "MapManager.h"

// include iostream where it's needed
#include <iostream>
// don't need ctor or dtor

void MapManager::DrawMap()
{
    // as khatharr said, but without the evil braceatendofstatement style :p
    for(auto& line : map)
    {
       std::cout << line << "\n";
    }
}

// nope nope nope nope nope
//MapManager &MapManager::GetInstance()
//{
//	MapManager temp;
//	return temp;
//}

and using it

MapManager map;
map.DrawMap();

so much cleaner... less code, less bugs... easy to use, hard to abuse.

 

 

Should I still not do this even if many other classes needs MapManager?

(sorry for the super late reply...)


Please.

for(auto& line : map) {
  std::cout << line << "\n";
}

Please.

 

Also, before you 'fix' the Meyers Singleton, why does this class need to be a singleton?

 

I made it a singleton since it was gonna be used by many other classes, figured it would be better that way than sending references everywhere.

Edited by BiiXteR

Share this post


Link to post
Share on other sites
A singleton is rarely a good solution. In a well-designed program you typically don't have that many dependencies, so perhaps you need to rethink your design. Sending the references everywhere at least makes the dependencies explicit. Otherwise you'll try to reuse some piece of code for a new project and you'll realize that the whole project has become one monolithic mess and you can't do it.

If you end up using global state of some sort, what is the advantage of using a singleton over using a global variable?

Share this post


Link to post
Share on other sites

Should I still not do this even if many other classes needs MapManager? (sorry for the super late reply...)

 
 
You've fallen for the most common misuse of the singleton pattern. The singleton is there is ensure a single instance of an object, not a single point of access.
 
Everyone who misuses a singleton in this fashion says the same thing: "I don't want to pass my X/Manager/God/Controller/Whatever object everywhere". As @Álvaro said, at least then your dependency is obvious.
 
If you find that you're passing a certain object everywhere down through layers of code, this is a code smell and a sign that you should refactor your code.
 
That said, sometimes a global really is the lesser evil. Things like a log object for example.
 
But let's say you look at your options and you decide that it really would be less painful to not pass MapManager everywhere. In that case, just create a global and own that decision. 
 
You have you MapManager class defined as normal (MapManager.h/.cpp). These files should know nothing about the global, otherwise you can't use them without it.
Create a header where you will declare your global MapManager
 
Globals.h

#include "MapManager.h"

// namespaces are great!
namespace InsertGameNameHere
{
   namespace Globals
   {
      // the global map state
      extern MapManager TheMap;
   }
}

Then in your main.cpp instantiate TheMap

#include "Globals.h"

// instantiate the map. this should be in exactly one place in the code
MapManager InsertGameNameHere::Globals::TheMap;

int main()
{
   InsertGameNameHere::Globals::TheMap.DrawMap();
   // do more stuff
}

Anywhere else you need to access the map, just include Globals.h without instantiating the map.

Monster.cpp

#include "Globals.h"

namespace InsertGameNameHere
{
    void SpawnMonster()
    {
       Monster monster;
       monster.SetPosition(Globals::TheMap.GetRandomSpawnPoint());
       // etc.
    }
}
  

Share this post


Link to post
Share on other sites

 

Should I still not do this even if many other classes needs MapManager? (sorry for the super late reply...)

 
 
You've fallen for the most common misuse of the singleton pattern. The singleton is there is ensure a single instance of an object, not a single point of access.
 
Everyone who misuses a singleton in this fashion says the same thing: "I don't want to pass my X/Manager/God/Controller/Whatever object everywhere". As @Álvaro said, at least then your dependency is obvious.
 
If you find that you're passing a certain object everywhere down through layers of code, this is a code smell and a sign that you should refactor your code.
 
That said, sometimes a global really is the lesser evil. Things like a log object for example.
 
But let's say you look at your options and you decide that it really would be less painful to not pass MapManager everywhere. In that case, just create a global and own that decision. 
 
You have you MapManager class defined as normal (MapManager.h/.cpp). These files should know nothing about the global, otherwise you can't use them without it.
Create a header where you will declare your global MapManager
 
Globals.h

#include "MapManager.h"

// namespaces are great!
namespace InsertGameNameHere
{
   namespace Globals
   {
      // the global map state
      extern MapManager TheMap;
   }
}

Then in your main.cpp instantiate TheMap

#include "Globals.h"

// instantiate the map. this should be in exactly one place in the code
MapManager InsertGameNameHere::Globals::TheMap;

int main()
{
   InsertGameNameHere::Globals::TheMap.DrawMap();
   // do more stuff
}

Anywhere else you need to access the map, just include Globals.h without instantiating the map.

Monster.cpp

#include "Globals.h"

namespace InsertGameNameHere
{
    void SpawnMonster()
    {
       Monster monster;
       monster.SetPosition(Globals::TheMap.GetRandomSpawnPoint());
       // etc.
    }
}
  

 

Alright, I'll go ahead and replace my singletons with that instead.

I'll read up some more on singletons as well.

Thanks for the help :)

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