dearimgui / dear_bindings

C header (and language binding metadata) generator for Dear ImGui
MIT License
221 stars 12 forks source link

No constructors/destructors are being generated for ImVector #3

Closed emoon closed 1 year ago

emoon commented 2 years ago

Currently no C wrappers are being generated (from what I can see at least) for ImVector_* This is not a big problem when you only get a vector from internal data, but it's worse in the cases like this

    IMGUI_API void  BuildRanges(ImVector<ImWchar>* out_ranges);                 // Output new ranges

Where the user will pass in a vector to get filed and then needs to destroy it. It's of course possible to manually call IM_FREE(..) on the data, but I think it's better to keep the implementation of it internally so it can be changed without the user needing to worry.

emoon commented 2 years ago

In this case I would expect the (C) user to write something like this:

ImVector_ImWchar ranges;
ImVector_<ImWchar>_construct(&ranges);
ImFontGlyphRangesBuilder_BuildRanges(.., &out_ranges);
// use out_range here
ImVector_<ImWchar>_destruct(&ranges);

Above it may be possible to remove the <ImWchar> and use common one as it will only setup some initial data and the last call will call free (and clear some data)

ocornut commented 2 years ago

Good point, when we decided to remove them (discussions from https://github.com/ocornut/imgui_private/issues/11#issuecomment-804267018 then removed on may 15), we forgot this ImFontGlyphRangesBuilder_BuildRanges() function which is an exception. AFAIK this is the only place in the public API where used is expected to construct a ImVector.

ImVector<> is a little odd since by design it doesn't map to std::vector: it does not destruct but has those helper functions:

~ImVector()           { if (Data) IM_FREE(Data); } // Important: does not destruct anything
void clear()          { if (Data) { Size = Capacity = 0; IM_FREE(Data); Data = NULL; } }  // Important: does not destruct anything
void clear_delete()   { for (int n = 0; n < Size; n++) IM_DELETE(Data[n]); clear(); }     // Important: never called automatically! always explicit.
void clear_destruct() { for (int n = 0; n < Size; n++) Data[n].~T(); clear(); }           // Important: never called automatically! always explicit.

We support and guarantee support for zero-init, so technically we could make that the defacto way to initialize in C (but I forgot if ImVector_ImWchar ranges = {}; works in C?) but may not be very forward thinking.

My suggestion is that we implement two template-type agnostic functions:

ImVector_construct(void* im_vector) { memset(...); }
ImVector_destruct(void* im_vector); // Important: clears memory does not destruct objects (if they have destructor)

Right now the only use case is ImFontGlyphRangesBuilder_BuildRanges(). If in the future if the C api needs to access destructors then we'll be able to add:

ImVector_SomeType_destruct(ImVector_SomeType* im_vector)

But I don't foresee this to be needed.

ShironekoBen commented 2 years ago

Ah, yeah, that's an awkward corner-case, certainly!

ImVector_construct()/ImVector_destruct() seemed like a sensible way to deal with this, so I've added those to the API and also a little bit of documentation to point people towards them.

ShironekoBen commented 1 year ago

I'm going to close this for now, feel free to reopen if you are still running into problems.