2

I don't want to show you a book so i am going to simplify. I have a class called "Tile" that looks similar to this:

class Tile{
public:
   struct Type{ unsigned int uvIndex; ... };
   Tile(Tile::Type* tt){ tt_ = tt; ... }
   Type tt_ = nullptr;
   Tile* getNorthEast(){
       printf("this %p\n", this); //for debugging purposes
       /* calculation; "this" pointer is need for that  */
       return northEastTilePtr;
   }
};

I want to allocate many of them in one big array that will never move again (no need for a ) so i do that allocation manually

//Manual allocation to prevent constructor from being called
Tile* tiles = (Tile*)malloc(numTiles*sizeof(Tile));
if(tiles == nullptr) throw std::bad_alloc();

Since the Tile type is not known at first i cannot call new to do that. Now i have memory allocated, but no constructors called. The world generator runs and calls for each tile 

tiles[tileIndex] = Tile(tileTypePtr);

Now it should have created all objects with all types. If I render the scene and without calling getNorthEast(); I can see, that the types have been set correctly due to the uvIndex (that just specifies which section of the texture to render). So tt_ is set correctly and the constructor must have run correctly too. However! If i now call getNorthEast(); the printf statement tells me that the this pointer is 00000000. This messes up the calculation and results in a crash.

It probably is due to undefined behaviour, but I do not see what I am doing wrong... I need your help for that.

EDIT 1: So I have now had a look at placement new. I have changed the allocation and assignment to:

//allocation 
tiles = (Tile*)new char[numTiles*sizeof(Tile)];
//assignment
new (&tiles[tileIndex]) Tile(tileTypePtr);

However this is still a nullptr. This time the assignment should be a complete overwrite without undefined behaviour. The allocation does essentially the same thing as std::vector.reserve();

Additional Info: the getNorthEast() is called much later in the main loop. Right now is just initialisation. It should therefore have no impact on wether the object is constructed or not.

Edit 2: I have just re-tried using a vector. same result.

std::vector<Tile> tiles;
//allocation
tiles_.reserve(numTiles);
//construction for each index
tiles.emplace_back(Tile(tileTypePtr));

Since neither vector nor doing it manually worked even with your input, i am starting to suspect the cause is some that I have not mentioned. I will keep looking until I find something else, that could cause trouble.

GRASBOCK
  • 631
  • 6
  • 16
  • 9
    The `malloc` function only allocates memory, it *doesn't call the constructors!* Using unconstructed object in any way (including assigning to them as that invokes the objects assignment operator) leads to [*undefined behavior*](https://en.wikipedia.org/wiki/Undefined_behavior). Please [get a few good books to read](https://stackoverflow.com/questions/388242/the-definitive-c-book-guide-and-list/388282#388282) and learn how to avoid such problems. – Some programmer dude Oct 05 '18 at 12:19
  • 1
    `tiles[tileIndex] = Tile(tileTypePtr);` is wrong because `tiles[tileIndex]` uses an uninitialized object. You have to call the constructor first before you can assign to it. – melpomene Oct 05 '18 at 12:19
  • 1
    @Someprogrammerdude "*Now i have memory allocated, but no constructors called.*" I'm pretty sure OP already knows that. – melpomene Oct 05 '18 at 12:20
  • 1
    You need to provide the rest of your code, where you are calling getNorthEast – grungegurunge Oct 05 '18 at 12:22
  • 3
    `//Manual allocation to prevent constructor from being called`: why? This is weird. You should give an explanation. And yes, we need a [MCVE]. We need at least th code that calls `getNorthEast` – Jabberwocky Oct 05 '18 at 12:25
  • 1
    You should create your objects with "placement new": `new (&tiles[tileIndex]) Tile(tileTypePtr);`, not with assignment. – molbdnilo Oct 05 '18 at 12:27
  • 4
    *I want to allocate many of them in one big array that will never move again (no need for a ) so i do that allocation manually* -- And you know something? Most, if not all implementations of `std::vector` do the same thing, but properly by using `placement-new`. So you're reinventing the wheel, only that your reinvention is broken. – PaulMcKenzie Oct 05 '18 at 12:32
  • @PaulMcKenzie In this specific case, the `vector` might not work, there is no default constructor or copy or copy-assignment operators. You can't use `vector` with this class without modifications. – Fantastic Mr Fox Oct 05 '18 at 12:33
  • 1
    @FantasticMrFox You [can use](https://godbolt.org/z/M_PLCX) `std::vector` with such classes. Just not the parts of the API that would require default-constructable elements. – Max Langhof Oct 05 '18 at 12:42
  • 1
    Take a look at [Malloc and constructors](https://stackoverflow.com/questions/2995099/malloc-and-constructors) and [Using malloc instead of new, and calling the copy constructor when the object is created](https://stackoverflow.com/questions/4956249/using-malloc-instead-of-new-and-calling-the-copy-constructor-when-the-object-is) to see how to call a ctor on manually allocated memory. – t.niese Oct 05 '18 at 12:44
  • @MaxLanghof That depends on the compiler. G++ 4.4.2 on QNX compiles the functions for `reserve` and `push_back` even if you don't use them. Meaning it will fail to compile even if you don't use the API. An obscure case, but it is sometimes what stops me from being able to use `std::vector`. – Fantastic Mr Fox Oct 05 '18 at 13:00
  • @FantasticMrFox why would there not be copy assignment operators for `Tile`? – eerorika Oct 05 '18 at 13:03
  • Ok,"Using unconstructed object in any way (including assigning to them as that invokes the objects assignment operator) leads to undefined behavior." that is new to me. I always thought the constructor returns an Object and assigning is just overwriting memory. I will definitly try the placement new, as it is pretty much exactly what i have been looking for. I don't really want to use a vector, because its just not needed. I do understand though, why you recommend it so strongly, though i don't think the vector is the solution;I will try some of your answers and get back maybe with some edits. – GRASBOCK Oct 05 '18 at 13:04
  • @user2079303 There are (i was wrong), but maybe there shouldn't be. More important there is no default constructor so `resize` and `push_back` couldn't work. My point is mostly that `vector` is super useful, maybe even in this case, but it is not always the answer when the classes are complex. – Fantastic Mr Fox Oct 05 '18 at 13:18
  • @FantasticMrFox 1. Why shouldn't there be? 2. `push_back` doesn't require default constructor. Sure, there are cases where vector won't work, but those are rare and obscure and I see no reason why this would be one of them. – eerorika Oct 05 '18 at 13:25
  • @FantasticMrFox Indeed, the requirements have changed in C++ 11 - requirements for the elements depend on which functions you use. Before C++ 11, copy-assignability/constructability was required. I believe g++ 4.4.2 would correspond to the latter. – Max Langhof Oct 05 '18 at 14:02

1 Answers1

4
tiles[tileIndex] = Tile(tileTypePtr);

What this does is it creates a temporary Tile instance, and then (move-) assigns that temporary into the Tile instance that (is supposed to) exists in the array. But that Tile instance doesn't exist, so the behaviour of the program is undefined.

Here is a correct way to construct an object into a pre-existing memory buffer:

 // you can use malloc, but I see no reason to need it
 unsigned char *buffer = new unsigned char[numTiles*sizeof(Tile)];
 auto offset = sizeof(Tile) * tileIndex;
 Tile* tptr = new(buffer + offset) Tile(tileTypePtr);

This syntax that reuses the memory for a new object is called "placement new". You need to include the <new> header to use it.

However, there is no need to re-implement this buffer reuse yourself. std::vector<Tile> does it for you, in addition to taking care of the hazardous memory management as well as the subtleties of exception safety and pointer aliasing rules. You've already tripped onto one caveat. There is no need to jump onto the next one. std::vector is usually the ideal solution for dynamic arrays of non-default-constructible types.

Edit: The answer below is based on my initial interpretation that you intend to have objects of different types in your array. I now notice that you have a Type pointer in your object and figure that you might actually be storing only Tile instances with different internal Type pointer (I assume that tt_ is supposed to be a pointer).

However, your structure seems a bit shaky. Have you considered how does the user of the array know what type of Tile you have constructed in which index? Have you considered that all objects in the array must have identical sizes and alignment requirements? None of those considerations can be enforced by the compiler.

If you know the list of possible tile types beforehand, I suggest using:

// let Tile1, Tile2, Tile3 be the possible types
std::vector<std::variant<Tile1,Tile2,Tile3>>

If you cannot constrain the list of types, then you can use:

std::vector<std::any>
eerorika
  • 232,697
  • 12
  • 197
  • 326
  • Ok, I have tried the placement new. I have also added it into an Edit to show how it looks. It should essentially be the same. However `this` is still a nullptr. Originally I was using a vector (reserve() for the allocation and emplace_back() for construction), but since this issue appeared i tried to go a bit more low level so I can have a better look at what is happening. – GRASBOCK Oct 05 '18 at 13:33
  • Your assumption about the tt_ is correct. The index is known from pointer subtraction. I fact: this is what the "this" pointer is used for. `unsigned int tileIndex = (unsigned int)(this - tiles)`. That worked when I was using C (i moved the program to c++ for better functionality). I should add that the tiles variable is global, so it can be accessed by the Tile functions – GRASBOCK Oct 05 '18 at 13:34
  • @RIJIK I'm pretty sure that your new code in the question violates pointer aliasing rules and thus UB. That said, I would guess that the this-null problem might be caused by some other UB somewhere else in your program. – eerorika Oct 05 '18 at 13:36
  • @ user2079303 You might be correct with it being caused by something else. What do you mean by "pointer aliasing rules" and what is wrong with my code? Trying to see any potential problems. – GRASBOCK Oct 05 '18 at 13:53
  • @RIJIK This is wrong: `(Tile*)new char[numTiles*sizeof(Tile)]`. A `Tile` pointer is not allowed to alias `char` objects. – eerorika Oct 05 '18 at 13:56
  • Even though i am not using the tiles until i overwrite the data? Its only temporary so it shouldn't be an issue. – GRASBOCK Oct 05 '18 at 15:17
  • @RIJIK yea, I'm not quite certain whether that's wrong. It might be OK. The pointer aliasing rules are quite tricky. – eerorika Oct 05 '18 at 15:38