2

This question might be a little stupid but I'm still fairly new to C++ and it's been a while since I've last done something with it.

I have a class called LEDBitmap that is supposed to hold the width, height and data of a bitmap with only ones and zeros.

In the header file I have the following struct:

struct MapData
{
  uint8_t width;
  uint8_t height;
  uint8_t[][] data;
};

As well as the following constructor, destructor and member variable:

class LEDBitmap
{

  public:
    LEDBitmap(uint8_t width, uint8_t, height, uint8_t data[][]);
    LEDBitmap(uint8_t width, uint8_t, height);
    virtual ~LEDBitmap() {   };

  [...]

  private: //members
    MapData _map;
};

I now want to write the constructors and possibly the destructor and so far I have the following for the first constructor:

//initialize an empty bitmap with only zeros in it
LEDBitmap::LEDBitmap(uint8_t width, uint8_t, height) {
  _map.width = width;
  _map.height = height;
  _map.data = new uint8_t[width][height];
}

Would this implementation work? (probably not) And should I bother actually implementing the destructor?

EDIT: Adjusted my code according to @gsamaras's suggestion. _map used to be *_ptr before.

EDIT: a friend suggested to use calloc() instead. I thus now have:

LEDBitmap::LEDBitmap(uint8_t width, uint8_t height) {
  _map.width = width;
  _map.height = height;
  _map.data = calloc(width*height*(sizeof(uint8_t));
}

and

class LEDBitmap
{

  public:
    LEDBitmap(uint8_t width, uint8_t, height, uint8_t data[][]);
    LEDBitmap(uint8_t width, uint8_t, height);
    virtual ~LEDBitmap() {
      free(_map.data);
    };

  private: //members
    MapData _map;

};
Lithimlin
  • 542
  • 1
  • 6
  • 24

1 Answers1

1

Since ptr is a pointer, no. You are attempting to fill the fields of a struct, that has not even memory allocated to it. That causes Undefined Behavior.

  1. You should first allocate memory for the struct, and then populate it.
  2. Then, in the destructor, you must de-allocate that memory.

Remember, when new is used, then delete must also be used. In general, you want to call delete exactly as many times as new was called.

But why use a pointer? It seems redundant in that case. And when you use pointers without a good reason, you make your code prone to errors.

Here are some suggestions you can pick from, instead of using a pointer (which wouldn't require you to define a constructor):

  • Use std::vector<uint8_t> to do all the work under the hood (related), as NathanOliver stated.
  • Use struct MapData as the data member instead of the pointer. That makes sense in OOP programming, if you want the struct to be re-used by another class for example.
  • If the struct is meant to be used only from this class, then consider giving the class directly the fields of the structs, as its data members, and not the struct itself.
gsamaras
  • 71,951
  • 46
  • 188
  • 305
  • Unfortunately, I've never learned how to properly allocate in C++. I know it has to be done but always get mixed up when and how it happens. – Lithimlin May 30 '18 at 14:04
  • Memory management is important in C++. Would you like me to include an example in my answer? Since it seems that STL makes you worried about the micro controller (where I do not know what is best). @Lithimlin – gsamaras May 30 '18 at 14:07
  • Yes, that would be awesome. – Lithimlin May 30 '18 at 14:08
  • 2
    @Lithimlin I could do that and continue my day, but now that I am thinking about it, it's not needed. Why would you need to use a pointer, and not the struct itself as a data member of the class? Assuming you are not going for the vector approach, which is suggested. – gsamaras May 30 '18 at 14:10
  • Alternatly, why a separate `MapData` type, instead of just having those members be part of `LEDBitmap` – Caleth May 30 '18 at 14:14
  • @gsamaras now that you mention it... I'm not so sure anymore. I used a different class before but I can't use that anymore because I'm using a different controller and now have different libraries and just started trying to emulate the class I'd used so far. They probably had some reasons for that, but you're right, I don't have any. – Lithimlin May 30 '18 at 14:15
  • @Lithimlin I see, thus I updated my answer, hope it helps now. – gsamaras May 30 '18 at 14:20
  • @Caleth good point, I updated my answer, what do you think, did it improve? – gsamaras May 30 '18 at 14:20
  • "when new is used, then delete must also be used" - and don't forget that when new[] is used then delete[] must be used rather than just using delete. – Sean Burton May 30 '18 at 16:19