Bug? array.set_free_when_destroyed and array.set_pointer

[MasterD]

I don't know, if this should be considered as a bug, therefore I wanted to post here first, for discussion. So, when using the irrlicht array type and setting

array.set_free_when_destroyed(false)

meaning
//! Sets if the array should delete the memory it uses upon destruction.
/** \param f If true, the array frees the allocated memory in its
destructor, otherwise not. The default is true. */

and afterwards calling
array.set_pointer(p1, count)
array.set_pointer(p2, count)

The 2nd call will set the array data p2 but will also delete the p1 content.

So, when looking at the docu, this behavior is not direct illegal, as the array isn't destructed.

But in my opinion this behavior (and docu) should be extended that calls like this (set_pointer and operator= ) won't destruct the data.

I haven't thought about this to the end, but in my use case that's what I want. So what do you think?

[vitek]

If set_pointer() doesn't deallocate the memory, then the user would have to manually delete it before calling set_pointer(), even if the memory was allocated by the constructor. Same for operator=().

Requiring this usage makes the core::array<> more difficult to use. Updating the code to work like this would cause breakage for user code. The change would be source and binary compatible, but the behavior change would result in memory leaks in currently working applications.

To be quite honest, the set_pointer() method should not exist on the array at all. That said set_pointer() should probably obey the free_when_destroyed flag. If that worked, you could set_pointer() to NULL before invoking operator=.

Travis

[MasterD]

Sorry for not being clear, in what I said.

With

But in my opinion this behavior (and docu) should be extended that calls like this (set_pointer and operator= ) won't destruct the data.

I meant more precisely:
... won't destruct the data, if the user had set
array.set_free_when_destroyed(false)

So, it won't break the code, since set_free.. is default true. Therefore, it will only result in a memory leak, if the user had set this flag to true and resets the pointer, without taking care.

Additionally, this mentioned mal-function (imho), will occur at several locations within irrArray, if set_pointer was used (with shared data). This is everywhere, where data is destructed / deallocated.

As it is in my use case very useful, I won't recommend removing set_pointer.

Here is my use case, to illustrate why set_pointer can be useful:

I'm rendering regular tesselated quadratic meshes (up to several houndreds)

There are at most 16 possible indices combinations.

In order to use hardware vertex/index buffers (SVN tree) I use a SMeshbuffer structure.

The vertex data is complete static and won't change over time.

The index data can change over time, to any of the 16 combinations.

Since I don't want to rebuild this index structure everytime needed, I like to call indexDataArray.set_pointer(my_prebuild_indices). Therefore having a really cheap resetting of the held data.

Of course this has to be copied to hardware, if hardware indices are used, but at least this won't copy the data twice (one time from my_prebuild_indices to indexDataArray and another one to the hardware buffer).

[hybrid]

IMHO the set_pointer was introduced for exactly the just mentioned usage - interpreting preallocated byte pools as arrays with management data kept elsewhere. Hence, I'd also favor to support this method, but I also see the needs for better support of the flag. Besides the two mentioned places there's also a destruct in reallocate. It seems to be complicated to find a proper support for the flag in this scenario. So I guess I'll add a check only to set_pointer and operator= for a first step.

[MasterD]

I'm not so sure about what to with that flag, too.

You could even kill that flag's current semantic and introduce another one, meaning:
"array is permitted to modify it's data structure layout"

if you understand what I mean.

With this semantic, other operations just like "realloc", "insert", "push_back", whatever, could be illegal, if that flag would be true.
Since whith your interpretation

interpreting preallocated byte pools as arrays with management data kept elsewhere.

this makes sense, since management is not to the array's concern, no?

It's a bit difficult to find a always working semantic and the corresponding answer of the code. With the above one, there are (at least) two working implementations of e.g. realloc:
1. Realloc is forbidden, call is rejected.
2. Realloc is allowed, the old data is copied (as it is currently), but the old data is not destructed/deleted AND the flag is set to false (else there would be a memory leak).

[hybrid]

Yes, you're right. I think I'd favor the second one, and then also resetting the flag free_when destroyed to true, because the new memory is to be handled by the array afterwards.
I guess it's becoming more and more confusing, the more of these extra stuff is introduced and used, but maybe we can at least fix the most common cases.