Pavlo: Hmm... Is this a security issue, or why did you restrict access to this ticket? And why did you add 28 people to the CC list here?

Sorry for this mess. By mistake, I checked "developer only" box and marked this bug as secure. Unfortunately, only developers can change the status of a bug. I was trying to fix this and sent email to so many people. If you can make this bug public - please help me with this.

I have never worked with libgdamm, and I will not discuss what shall or shall
not be added to libgdamm. This comment is about what might be added to glibmm,
in glibmm/tools/m4/member.m4.

_MEMBER_SET_CHAR
----------------
I think _MEMBER_SET_STRING or _MEMBER_SET_STR is a better name. If the member to
set is just one char (instead of a char*, pointing to a null-terminated string)
then _MEMBER_SET can be used.

It's unfortunate that the way to deallocate and to allocate the string are
specified in completely different places. The deallocation with g_free() is
determined by the _MEMBER_SET_CHAR macro. The allocation with g_strdup() is
determined by a _CONVERSION macro in another .m4 file or a .hg file.
What do you think about including g_strdup() in _MEMBER_SET_CHAR?

  void __CPPNAME__::set_$1(const $3`'& value)
  {
    g_free(gobj()->$2);
    gobj()->$2 = g_strdup(_CONVERT($3,$4,`value'));
  }

Then the used _CONVERSION macro must be something like

  _CONVERSION(`Glib::ustring',`gchar*',`$3.c_str()')

_MEMBER_SET_PTR_CHAR
--------------------
Is this macro useful? What do you intend to use it for?

Created attachment 369060 [details] [review]
New version of the patch

Thanks for the comments. I modified the patch and included some suggestions. I think it is better to use _MEMBER_SET_STR then ..._CHAR. If you think this patch will benefit glibmm, I will prepare a corresponding patch and provide as new bugreport or may attach here. In this case we probably don't need it in libgdamm.

Also, _CONVERSION(`Glib::ustring', `const gchar*', `($3).c_str()') is present in libgdamm macro file  tools/m4/convert_libgdamm.m4. Again, as to me this conversion should be moved to glibmm. 

Please let me know what would be the best next step. 
Thanks.

_MEMBER_SET_STR could be added to glibmm/tools/m4/member.m4. It would be useful
in glibmm/glib/src/optionentry.hg. But you still make it unnecessarily long.
g_free() does nothing it it's given a nullptr. It's unnecessary to test for
nullptr before g_free() is called. BTW, the same goes for the C++ delete
operator. I've seen lots of unnecessary tests of type
  if (p) delete p;
And setting gobj()->$2 = nullptr, when it's unconditionally set to something
else in the next statement is also unnecessary.

I still don't see the need for _MEMBER_SET_PTR_STR. Unless you can give a good
example of a case where it's useful, I don't recommend it for inclusion in
glibmm.

> Also, _CONVERSION(`Glib::ustring', `const gchar*', `($3).c_str()') is present
> in libgdamm macro file  tools/m4/convert_libgdamm.m4. Again, as to me this
> conversion should be moved to glibmm.

If it will be used in glibmm, it would fit there. But I'm not sure that
convert_glib.m4 would be the best location. It could be added to the .hg files
where it's needed. Then it would not be available in other modules. The
conversions for the_MEMBER_SET* macros are a bit special (as are some
conversions for _WRAP_SIGNAL and _WRAP_VFUNC). Such conversions are best kept
local to the files where they are used. Otherwise they can be used by mistake in
situations where they are wrong, and cause hard-to-find errors.


You can file a new bug for glibmm, if you like. If you don't intend to propose
changes in libgdamm, you can instead transfer this bug to glibmm by changing
Product from libgdamm to glibmm.

Created attachment 369131 [details] [review]
patch for glibmm master

Patch to push to glibmm master. I also added minimal comment to *.m4 file. Tested for libgdamm - works fine. The reason why people still check pointer before calling delete and g_free is because this is common in C. I also didn't pay attention to this. Thanks for hint. Initially, I used some code from the repo to see how other people are doing the same thing. It ma be a good idea to have a style guide where common mistakes discussed. Anyway, I will try to help as much as I can. Thanks for your help.

Created attachment 369157 [details] [review]
patch: member.m4: Add _MEMBER_SET_STR, setter for strings (glibmm)

This is a modification of your glibmm patch. I've replaced
  g_free(gobj()->$2);
by
  g_free((char*)gobj()->$2); // Cast away const, if any

I had to do that, or else _MEMBER_SET_STR can't be used in Glib::OptionEntry.

I've also changed your comments and commit message a bit, and moved
_MEMBER_SET_STR later in member.m4, where the other setters are defined.

If you accept this patch, I'll push it to glibmm. Or you can do it yourself,
if you have write permission to the git repository.

Works for me. I don't have writing permission to master. Just wandering, is the any difference to cast to char* vs gchar*. I know they are the same but still. Please push to master. I have another patch for libgdamm which depends on this one. Thanks.

Comment on attachment 369157 [details] [review]
patch: member.m4: Add _MEMBER_SET_STR, setter for strings (glibmm)

I have pushed the patch to glibmm, master branch and glibmm-2-54 branch.

There is no difference between casting to char* or to gchar*. In this case, it
would also have been possible to cast to void*. g_free() takes a gpointer,
which is void*. What's important is to cast away const, if gobj()->$2 is a
const char* or const gchar*.

Perfect! Thanks. I marked this bug as "resolved".

And I changed the resolution from NOTABUG to FIXED.