0

I have created a class template that holds a union of the following type, which should be of variable length.

union BufferUnion {
    int buf32;
    short buf16[2];
};

class SomeClass {
    
    int nBufs;
    int buflen;

    BufferUnion **bu;

    SomeClass(int nBufs, int buflen);
    ~SomeClass();
}

and now I want to dynamically allocate a few of these buffers in the .cpp file, but I am getting a read violation when trying to call the constructor.

SomeClass::SomeClass(int nBufs, int buflen)
{
    this->nBufs = nBufs;
    this->buflen = buflen;

    for (int i=0; i < nBufs; ++i) {
        bu[i] = new BufferUnion[buflen]; 
    } 
}
neolith
  • 699
  • 1
  • 11
  • 20
  • 3
    Tip: Don't. Use `std::vector`. – tadman Apr 22 '21 at 13:43
  • 2
    Tip: Use [constructor lists](https://en.cppreference.com/w/cpp/language/constructor). This side-steps the `this->` mess. – tadman Apr 22 '21 at 13:44
  • 1
    Hint: What does `bu[i] = ...` do on an uninitialized `bu`? – tadman Apr 22 '21 at 13:44
  • 3
    Your `bu` pointer is still uninitialized. But instead of doing all this, use a `std::vector` with a size of `nBufs * buflen` and map your 2D coordinates to 1D indices (ex. `i = x + y * nBufs`). – François Andrieux Apr 22 '21 at 13:44
  • 2
    Use a 1d vector, wrap it in a class, and pretend like it has multiple dimensions. That gives you ease of use and great performance. Also, FWIW, type punning through a union like I suspect you are using, is illegal in C++. I haven't seen a compiler not do what is expected, but it should be noted that you have opened the door to UB land. – NathanOliver Apr 22 '21 at 13:45
  • and don't use `union`, use `std::variant` instead. (and if it was for type punning, it was wrong anyway). – Jarod42 Apr 22 '21 at 13:54
  • Yeah but how to create a three dimensional vector. Seems messy to me.. – neolith Apr 22 '21 at 13:56
  • @neolith Using an `std::vector` you just multiple the size by the additional dimension's magnitude. It is significantly simpler than adding more nested loops. – François Andrieux Apr 22 '21 at 14:11
  • @neolith -- FYI -- If you don't use `vector` for some reason, then [see this answer](https://stackoverflow.com/questions/21943621/how-to-create-a-contiguous-2d-array-in-c/21944048#21944048). – PaulMcKenzie Apr 22 '21 at 14:14
  • So first declare ``std::vector bu;`` and then initialiaze it with ``bu = new std::vector * nBufs;``? This doesn't work – neolith Apr 22 '21 at 14:15
  • @neolith -- Stop using `new[]`. The whole purpose of `std::vector` is to not use `new[]`. C++ does not require `new` to create objects, unlike other languages you may have used before. If you want to go down the `new[]` road, then you better have a good reason for it, and know what you're doing (like the link I had in my previous comment). – PaulMcKenzie Apr 22 '21 at 14:17
  • C++ is really super confusing... -.- – neolith Apr 22 '21 at 14:17

1 Answers1

1

First thing you should do when creating the two-dimensional array is to allocate the memory for the first dimension, then iterate on it creating the second dimension. You've got the second part right, but what you're missing is: bu = new BufferUnion*[nBufs] before the loop creating the second dimension.

As a sidenote: What about the comma in for (int i=0; i < nBufs, ++i) instead of semicolon: for (int i=0; i < nBufs; ++i)? With comma separator the compiler will consider it as only one expression.

  • The comma was just a typo. Was correct in the actual code – neolith Apr 22 '21 at 13:57
  • @neolith Sure, no problem. That's why I wrote it as a side note. My intention was to point it in case of somebody just copy-paste this including this typo. – BartoszKlonowski Apr 22 '21 at 13:59
  • While this fixes the immediate problem, the code still has other problems which are likely to lead to pointer related errors. For example it doesn't obey the [rule of 3/5/0](https://en.cppreference.com/w/cpp/language/rule_of_three) is will likely crash if `SomeClass` is copied or assigned. This fix is accurate, but basically just postpones the problems. – François Andrieux Apr 22 '21 at 14:10
  • @FrançoisAndrieux Sure, but the purpose of this answer is to solve the problem describe in the post, not the whole code. – BartoszKlonowski Apr 22 '21 at 14:12
  • @FrançoisAndrieux Yeah, I heard of those rules, but C++ is so much to learn. I thought I would easily grasp this coming from C, but it is really a lot to learn by heart. – neolith Apr 22 '21 at 14:29
  • @neolith In that case, it is best to stick with the rule of 0 and embrace value semantics and RAII. If you have to write your own copy constructor, assignment operator or destructor or find yourself needing `malloc` or `new` then you are straying from the rule of 0. It is a sign you should look for another solution. Using `std::vector` is compatible with this strategy. Use it anytime you would be tempted to use `new[]` and use `std::make_unique` or `std::make_shared` any time you would use `new`. It greatly simplifies everything. `new` is basically never used in modern C++. – François Andrieux Apr 22 '21 at 14:42
  • @neolith *I thought I would easily grasp this coming from C* -- C is driving a car, C++ is driving a snowmobile. Just because you know how to drive a car doesn't mean you know or have good knowledge in learning to drive a snowmobile. – PaulMcKenzie Apr 22 '21 at 15:37