std::string causing crashes!

Started by
8 comments, last by Juliean 10 years, 4 months ago

In the past, I've gotten flak for using "unsafe" C-strings. In C++, std::string is giving me a headache. Randomly, when using a C++ string, it will cause my game to crash. Maybe it's something simple, but honestly, I've never had this problem before. It only happens when I try to set one string variable equal to another, like so:


void game_app_t::add_new_hint( std::string str )
{
    struct hint_t* h = (struct hint_t*) malloc( sizeof( struct hint_t ) );
    
    /* Add a new hint to the screen */
    h->pos = m_user.pos;
    h->str = str;
    h->timer = 60;
    h->alpha = 255.0f;
    h->pos.v[0] += 32.0f;
    
    list_add_beginning( &m_hints, h );
}

/* Example usage */
add_new_hint( "Avoid this" );

I've had this problem with random crashes before, and I ended up saying "screw it, I'll use C-strings", and that was the end of the crashes. Now it's happening again, and I'd rather find a solution as opposed to another work around fix. Any ideas? Thanks.

Shogun.

EDIT: Not sure if it matters, but I'm using XCode and this game is being dev'ed on Mac OSX Lion.

Advertisement

std::string has a non-trivial constructor, so malloc is out of the question. You need to allocate memory with new to ensure that the constructors of the hint_t members are executed, because malloc does not ensure that. You are basically assigning to an std::string that hasn't been constructed properly.

In addition to what Brother Bob said, never use malloc in C++. Not only does new ensure that constructors are called, it's also more type safe and the C++ way of allocating memory.

Try to avoid both `malloc' and `new'. If you are adding something to a container, use a standard container (of objects, not pointers) and don't even mention explicit memory allocation in your code.

Here's what a more idiomatic C++ version of your code could look like:
struct Hint {
  Vector2D pos;
  std::string str;
  int timer;
  float alpha;

  Hint(Vector2D const &pos, std::string const &str, int timer, float alpha)
    : pos(pos),
      str(str),
      timer(timer),
      alpha(alpha) {
  }
};

class GameApp {
  std::vector<Hint> hints;
  User user;

public:
  void add_new_hint(std::string const &str) {
    hints.push_back(Hint(user.pos + Vector2D(32.0f, 0.0f), str, 60, 255.0f));
  }
  
  // ...
};

Try to avoid both `malloc' and `new'. If you are adding something to a container, use a standard container (of objects, not pointers) and don't even mention explicit memory allocation in your code.

Here's what a more idiomatic C++ version of your code could look like:


struct Hint {
  Vector2D pos;
  std::string str;
  int timer;
  float alpha;

  Hint(Vector2D const &pos, std::string const &str, int timer, float alpha)
    : pos(pos),
      str(str),
      timer(timer),
      alpha(alpha) {
  }
};

class GameApp {
  std::vector<Hint> hints;
  User user;

public:
  void add_new_hint(std::string const &str) {
    hints.push_back(Hint(user.pos + Vector2D(32.0f, 0.0f), str, 60, 255.0f));
  }
  
  // ...
};

If this is running on a C++11 compiler I would use an emplace_back in this case because it will avoid the copy construction in push_back, but only in the case where you construct the object on the push_back call.

Worked on titles: CMR:DiRT2, DiRT 3, DiRT: Showdown, GRID 2, theHunter, theHunter: Primal, Mad Max, Watch Dogs: Legion


If this is running on a C++11 compiler I would use an emplace_back in this case because it will avoid the copy construction in push_back, but only in the case where you construct the object on the push_back call.

Hmmm... I tried that on g++ 4.7.1 and it doesn't seem to avoid the copy. Perhaps I am not doing it right? I compiled it with


g++ kk.cpp -o kk -O3 -Wall -Wextra -std=c++11

#include <iostream>
#include <vector>

struct Vector2D {
  float x, y;
 
  Vector2D(float x, float y) : x(x), y(y) {
  }
};

Vector2D operator+(Vector2D const &v1, Vector2D const &v2) {
  return Vector2D(v1.x + v2.x, v1.y + v2.y);
}

struct Hint {
  Vector2D pos;
  std::string str;
  int timer;
  float alpha;

  Hint(Vector2D const &pos, std::string const &str, int timer, float alpha)
    : pos(pos),
      str(str),
      timer(timer),
      alpha(alpha) {
  }
 
  Hint(Hint const &other)
    : pos(other.pos),
      str(other.str),
      timer(other.timer),
      alpha(other.alpha) {
    std::cerr << "A hint is being copied!\n";
  }
};

struct User {
  Vector2D pos;
 
  User(Vector2D const &pos) : pos(pos) {
  }
};

class GameApp {
  std::vector<Hint> hints;
  User user;

public:
  void add_new_hint(std::string const &str) {
    hints.emplace_back(Hint(user.pos + Vector2D(32.0f, 0.0f), str, 60, 255.0f));
  }
 
  GameApp(User const &user) : user(user) {
  }
};

int main() {
  GameApp game_app(User(Vector2D(1.0f, 2.0f)));
  game_app.add_new_hint("A hint!");
}

EDIT: Oh, I got it. If there is a move constructor, it does use it. I added this one and it works fine:


  Hint(Hint &&other)
    : pos(std::move(other.pos)),
      str(std::move(other.str)),
      timer(std::move(other.timer)),
      alpha(std::move(other.alpha)) {
    std::cerr << "A hint is being moved!\n";
  }

FURTHER EDIT: This also works:

  Hint(Hint &&) = default;


If this is running on a C++11 compiler I would use an emplace_back in this case because it will avoid the copy construction in push_back, but only in the case where you construct the object on the push_back call.

Hmmm... I tried that on g++ 4.7.1 and it doesn't seem to avoid the copy. Perhaps I am not doing it right? I compiled it with

emplace_back forwards the parameters to a constructor, so if you construct a Hint object and pass it to emplace_back, it would then have to pass that object to the copy constructor. This one does not invoke the copy constructor in VS2012.

hints.emplace_back(user.pos + Vector2D(32.0f, 0.0f), str, 60, 255.0f);


If this is running on a C++11 compiler I would use an emplace_back in this case because it will avoid the copy construction in push_back, but only in the case where you construct the object on the push_back call.

Hmmm... I tried that on g++ 4.7.1 and it doesn't seem to avoid the copy. Perhaps I am not doing it right? I compiled it with

emplace_back forwards the parameters to a constructor, so if you construct a Hint object and pass it to emplace_back, it would then have to pass that object to the copy constructor. This one does not invoke the copy constructor in VS2012.


hints.emplace_back(user.pos + Vector2D(32.0f, 0.0f), str, 60, 255.0f);

I see. That works on my compiler as well.

By the way, push_back won't make a copy either if you provide a move constructor.

  Hint(Hint &&) = default;

Okay, now I get it. It's amazing what I was never taught in my C++ courses! ^o^

My linked list library was originally written in pure C, and I was too lazy to change malloc/free to new/delete. I usually prefer to avoid using std C++ libraries in my code, but I made an exception this time, and it turned out to be a major mistake portability wise (I'll never forget the nightmare I had dealing with std C++ and Blackberry OS). Oh well.

Thanks.

Shogun.


By the way, push_back won't make a copy either if you provide a move constructor.
  Hint(Hint &&) = default;

That is still more wasteful than using emplace-back, since first a Hint-instance is created, and then the move constructor is called for the Hint-instance in the vector. emplace_back only calls the constructor once for the vector instance. Specially here, since all members are POD except for the std::string, having a move-constructor won't help much, move constructor normally only helps when custom memory allocations are made in the class. Not that the difference is a big deal here anyway, but just wanted to add this.

This topic is closed to new replies.

Advertisement