On 23.02.2015 10:04 Pierre Ossman wrote:
> On Sat, 21 Feb 2015 01:13:27 +0100,
> Albrecht Schlosser wrote:
>
>> Hmm, are you sure? You should double check this. I looked into the code
>> today, but it's too confusing to grok it in a short time. However I saw
>> that there is a member Fl_Menu_::alloc that can be 0, 1, or 2 (despite
>> the comment that says only 0 or 1). Only if it is 2 are the strings
>> free'd. In Fl_Menu_add.cxx, line #384 it is set to 2 in
>> Fl_Menu_::insert() under certain conditions only. I'm not sure that all
>> is okay or as documented or expected, but I don't know if your concerns
>> are correct (but I don't dare saying they are not!).
>
> Urgh. You are right. That however just makes things even more
> confusing.
Yep. It's really confusing.
> Look at how insert() works. If the menu started off using menu() or
> copy() then it will keep alloc at 1. But the strings given to insert()
> are still strdup():ed. IOW they will leak.
I got that impression as well, but could not confirm it in the time I
spent to look at the code. I went in circles.
> And replace() is even more fun. It will strdup() or not depending on
> how the current menu was created. Which the caller might not have any
> idea about.
We can perhaps fix this by strdup()'ing everything once we have to copy
at least one menu text and then setting alloc to 2. This doesn't prevent
the initial strings from leaking, but makes all additional strings work
correctly (being copied and free'd). I wouldn't care much about the
initial strings "leaking" because they will most probably be strings in
static memory. In other cases we'd need the new API feature.
That means: if you construct a menu with static strings, don't care. If
you construct menu strings elsewhere (in local variables or malloc'd),
use the new API to make sure they are copied. In this case you can free
the memory or let the local variable go out of scope as soon as the menu
is created or copied etc.
In all other cases (legacy code) there can only be leaks of the initial
strings if they are alloc'd and not in static memory, which would be
negligible.
>>> So this is apparently the safe and correct way to use the API:
>>>
>>> Fl_Menu_Item popup[] = {
>>> {strdup("&alpha"), FL_ALT+'a', the_cb, (void*)1},
>>> {strdup("&beta"), FL_ALT+'b', the_cb, (void*)2},
>>> {strdup("gamma"), FL_ALT+'c', the_cb, (void*)3, FL_MENU_DIVIDER},
>>> {0}};
>>> menu->copy(popup);
>>>
>>> Beautiful.
>>
>> I'd check that again. Why copy? Why strdup? The menu is designed to be
>> left alone if given by the user, but copied internally when necessary
>> (i.e. when calling add()). I'd be surprised if this was so buggy, but
>> again: I'm really not sure, and I didn't write nor comprehend the menu
>> code fully.
>>
>
> copy() is most definitely needed since popup here lives on the stack
> and menu might live longer than that.
True.
But you didn't say that this was in a subroutine or function ;-)
> The strings didn't need to be
> duplicated here though as I didn't see the full extent of "alloc". If
> they were also on the stack they would have to be though, even if that
> means they would leak.
... and here comes the new method or option/argument into play. We need
something like Fl_Widget::copy_label() to make sure we can free strings
we constructed elsewhere (not in static memory) right after giving them
to the menu and we must make sure that static strings are not free'd.
>>> - Introduce a Fl_Menu_::copy2() with sane behaviour.
>>
>> Something like that might be useful anyway. Maybe a way to say: it's all
>> allocated data, use it and free it yourself (give ownership of strings
>> (labels) to FLTK as well, like copy_label()), i.e. force setting alloc
>> to 2 internally, or something like this.
>>
>
> I think that's really the only sane way for an API like this.
Maybe we can add another flag bit to each menu entry saying that _this_
menu string is copied and needs to be free'd instead of having only one
global flag. If that helps. So you could also malloc the string and set
the "allocated" bit for this string and let the menu code free it when
not needed any more.
After thinking more about it, I like this idea. One bit in the menu
array per string instead of one global member alloc == 2.
> It doesn't really solve my current issue though...
... and your current issue is? You started your description with
Fl_Menu_::add() and the problem of escaping '/', but I don't know
exactly what the underlying problem is. Could you elaborate?