After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 589197 - no way to have an arg callback for a Glib::OptionEntry
no way to have an arg callback for a Glib::OptionEntry
Status: RESOLVED FIXED
Product: glibmm
Classification: Bindings
Component: general
2.20.x
Other Linux
: Normal normal
: ---
Assigned To: gtkmm-forge
gtkmm-forge
Depends on: 642823
Blocks:
 
 
Reported: 2009-07-21 01:39 UTC by Hubert Figuiere (:hub)
Modified: 2011-02-23 15:40 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch: Add OptionGroup::add_entry() that takes a slot with callback function. (22.66 KB, patch)
2011-02-14 15:36 UTC, Kjell Ahlstedt
none Details | Review
glibmm_optiongroup_argcallback_warning.patch (2.11 KB, patch)
2011-02-16 10:58 UTC, Murray Cumming
none Details | Review
patch: OptionGroup: Fix build error, remove memory leak. (2.95 KB, patch)
2011-02-17 10:31 UTC, Kjell Ahlstedt
none Details | Review

Description Hubert Figuiere (:hub) 2009-07-21 01:39:31 UTC
Following bug 588989,

The optional arg flag is actually only for when we pass a callback to parse the option (G_OPTION_ARG_CALLBACK). 

In Glibmm there is no way to specify this to any of the Glib::OptionGroup::add_entry() overrides.
Comment 1 Kjell Ahlstedt 2011-01-27 19:30:09 UTC
I might make a patch that adds the missing OptionGroup::add_entry(), and I
would appreciate comments on my proposed API before I finish the implementation.

My first idea of a new OptionGroup::add_entry() method looks like this:

  /** For example bool on_option_arg(const Glib::ustring& option_name,
      const Glib::ustring* value);.
   */
  typedef sigc::slot<bool, const Glib::ustring&, const Glib::ustring*>
    SlotOptionArg;

  void add_entry(const OptionEntry& entry, const SlotOptionArg& slot);

On second thought, that's not ideal. Two objections:

1. The callback function that goes into SlotOptionArg takes a
   'const Glib::ustring* value' argument. Why not 'const Glib::ustring& value'?
   Because the C callback, GOptionArgFunc, can get a null pointer. There is a
   difference between no value, and a value consisting of an empty string.
   So, would you prefer the following solution?
   
   /** For example bool on_option_arg(const Glib::ustring& option_name,
       const Glib::ustring& value, bool has_value);.
    */
   typedef sigc::slot<bool, const Glib::ustring&, const Glib::ustring&, bool>
     SlotOptionArg;

   void add_entry(const OptionEntry& entry, const SlotOptionArg& slot);

2. The value can be a string encoded either in UTF-8 or in the encoding used
   for filenames. In case of filename encoding, std::string should be used
   instead of Glib::ustring. Then it could look like this:
   
   /** For example bool on_option_arg(const Glib::ustring& option_name,
       const Glib::ustring& value, bool has_value);.
    */
   typedef sigc::slot<bool, const Glib::ustring&, const Glib::ustring&, bool>
     SlotOptionArgString;
   
   /** For example bool on_option_arg(const Glib::ustring& option_name,
       const std::string& value, bool has_value);.
    */
   typedef sigc::slot<bool, const Glib::ustring&, const std::string&, bool>
     SlotOptionArgFilename;

   void add_entry(const OptionEntry& entry, const SlotOptionArgString& slot);
   void add_entry_filename(const OptionEntry& entry,
                           const SlotOptionArgFilename& slot);

   This solution does not mimic the glib API as closely as the present
   add_entry() methods do. Is that a bug or a feature? The glib API is a bit
   inconsistent here. The choice between UTF-8 and filename encoding is done
   with G_OPTION_ARG_STRING, G_OPTION_ARG_FILENAME, G_OPTION_ARG_STRING_ARRAY,
   and G_OPTION_ARG_FILENAME_ARRAY, which in glibmm are set by
   OptionGroup::add_entry[_filename]. But when a callback function is chosen,
   there is only G_OPTION_ARG_CALLBACK, and the encoding is chosen with
   G_OPTION_FLAG_FILENAME. Option flags are set by OptionEntry::set_flags().

   It would be no problem to let the two new add_entry[_filename] methods
   ignore the G_OPTION_FLAG_FILENAME in the supplied OptionEntry, and set the
   GOptionFlags to suit glib, because add_entry[_filename] stores its own copy
   of the OptionEntry in the OptionGroup.


Another question: Hubert, did you ever file a glib bug in connection to
bug 588989? The only similar bug I can find is bug 577727, and it was filed by
someone else and quickly closed without action.

g_option_context_parse() could easily be made to behave better when
G_OPTION_FLAG_OPTIONAL_ARG is used inappropriately. When a short option
(like -s) is parsed, and G_OPTION_ARG_CALLBACK is not specified, then
G_OPTION_FLAG_OPTIONAL_ARG is ignored. When a long option (like --search) is
parsed, the program ends with 'assertion failed'. That's not necessary,
and not nice.
Comment 2 Murray Cumming 2011-01-28 08:47:51 UTC
(In reply to comment #1)
> 1. The callback function that goes into SlotOptionArg takes a
>    'const Glib::ustring* value' argument. Why not 'const Glib::ustring& value'?
>    Because the C callback, GOptionArgFunc, can get a null pointer. There is a
>    difference between no value, and a value consisting of an empty string.

Not a very useful difference. I'd prefer to avoid allowing that distinction, to avoid forcing people to deal with the memory management of a ustring*.

>    It would be no problem to let the two new add_entry[_filename] methods
>    ignore the G_OPTION_FLAG_FILENAME in the supplied OptionEntry, and set the
>    GOptionFlags to suit glib, because add_entry[_filename] stores its own copy
>    of the OptionEntry in the OptionGroup.

That seems worth a try then. Please make sure that it's explained in comments in the code.
Comment 3 Kjell Ahlstedt 2011-02-06 19:34:57 UTC
Then I'll aim at implementing the suggestion in comment 1, section 2.

There are a number of TODO comments in the OptionGroup source code. I feel
tempted to fix most of those while I'm adding the new add_entry[_filename],
and also to make other changes.

virtual bool on_pre_parse(OptionContext& context, OptionGroup& group);
virtual bool on_post_parse(OptionContext& context, OptionGroup& group);
virtual void on_error(OptionContext& context, OptionGroup& group);

in Glib::OptionGroup are the glibmm analogs of

typedef gboolean (*GOptionParseFunc) (GOptionContext *context,
                  GOptionGroup *group, gpointer data, GError **error);
typedef void (*GOptionErrorFunc) (GOptionContext *context,
              GOptionGroup *group, gpointer data, GError **error);

1.
The GError **error argument in GOptionErrorFunc is an input argument that
contains information on an error that has occurred before the function is
called. The prototype of on_error() should be

virtual void on_error(OptionContext& context, Error& error);

2.
The GError **error argument in GOptionParseFunc is an output argument, and
the functions that call on_pre_parse() and on_post_parse(), i.e. the local
functions g_callback_pre_parse() and g_callback_post_parse() in
optiongroup.ccg, should set it when on_pre_parse() and on_post_parse() throw
an exception.

***Question: Is it still necessary to wrap try and catch statements in
   #ifdef GLIBMM_EXCEPTIONS_ENABLED
   #endif //GLIBMM_EXCEPTIONS_ENABLED
A line in configure.ac suggests that GLIBMM_EXCEPTIONS_ENABLED is always
defined now and in the foreseeable future.

3.
In all of on_pre_parse(), on_post_parse(), on_error() the OptionGroup& group
argument is useless. It's always equal to *this. It should be removed.

4.
on_pre_parse(), on_post_parse() and on_error() are public. They are
callback functions, called while Glib::OptionContext::parse() is executing.
Most such callback functions in other classes are protected.
The real callback functions (called from GTK+) are implemented as local
functions in optiongroup.ccg. Therefore it's probably necessary to make the
on_* functions public. If you let GTK+ call protected static member functions,
it's possible to make on_* protected. This technique is used when gmmproc
generates callback functions for signals and virtual functions.
(Also discussed in bug 588988, comment 4.)

5.
optiongroup.hg contains the line
#include <glib.h> //TODO: Try to hide this.

It's a good ambition to hide glib.h from glibmm users, but is it worth doing?
Considering there are 26 header files in glibmm/glib/glibmm that include
glib.h, one of which is the ubiquitous ustring.h?
I'd rather remove this TODO comment.

6.
A comment in optionentry.hg says that enum Flags is copied instead of
generated, so it can be put inside the class. Is this an obsolete comment?
Other classes include enums inside the class with _WRAP_ENUM. See for instance
date.hg. I guess _WRAP_ENUM should be used in optionentry.hg, if possible.
Bug 86864 suggests that it might not be possible after all.

7.
One OptionGroup issue is handled separately in bug 588988.
Comment 4 Kjell Ahlstedt 2011-02-10 08:57:21 UTC
Ah, comment 3 rests on a false assumption.
I took it for granted that now is a good time to implement API/ABI breaking
changes for glibmm 3. But I was wrong. gtkmm 3 will not be accompanied by
glibmm 3. Only API/ABI preserving changes can be made to glibmm.

Let's see what can still be implemented of all my suggestions in comment 1.

0. (new add_entry with arg callback) Yes, probably.
1. No
2. Yes, probably.
   An application program's reaction to an exception thrown by on_pre_parse
   or on_post_parse will change. Is it an API break?
3. No
4. No
5. Yes, if the solution is to just delete a comment.
6. Requires a fix of gmmproc (bug 86864).
   The declaration of enum Flags also (by mistake?) declares the public data
   member Glib::OptionEntry::Flags GOptionFlags.
   _WRAP_ENUM, when applied to a flags type, generates operators that are meant
   to be global. The compiler doesn't accept them in a class declaration.
   There are no such Flags operators now, so we can probably do without them,
   but there is way to make _WRAP_ENUM generate only the enum Flags declaration
   but no operators.
7. See bug 588988.
Comment 5 Kjell Ahlstedt 2011-02-10 08:59:39 UTC
The reference to comment 1 in comment 4 is wrong. Shall be comment 3.
Comment 6 Murray Cumming 2011-02-10 09:04:37 UTC
This is a bit spaghetti-like now. Could you just state the current situation in a self-contained way, please?
Comment 7 Kjell Ahlstedt 2011-02-10 12:16:36 UTC
You are perfectly right.
Here's what I now suggest I shall do.

A. Add another add_entry() and an add_entry_filename() to Glib::OptionGroup.
   This is what this bug report is about from the beginning.

   /** For example bool on_option_arg(const Glib::ustring& option_name,
       const Glib::ustring& value, bool has_value);.
   */
   typedef sigc::slot<bool, const Glib::ustring&, const Glib::ustring&, bool>
     SlotOptionArgString;
   
   /** For example bool on_option_arg(const Glib::ustring& option_name,
       const std::string& value, bool has_value);.
   */
   typedef sigc::slot<bool, const Glib::ustring&, const std::string&, bool>
     SlotOptionArgFilename;

   void add_entry(const OptionEntry& entry, const SlotOptionArgString& slot);
   void add_entry_filename(const OptionEntry& entry,
                           const SlotOptionArgFilename& slot);

B. If it's not considered an API breaker:
   In g_callback_pre_parse() and OptionGroup::post_parse_callback(), transfer
   an exception thrown by OptionGroup::on_pre_parse() or
   OptionGroup::on_post_parse() to the 'GError** error' output argument.
   Then it will show up as an exception thrown by OptionContext::parse(),
   instead of stopping the program.

   Is it still necessary to wrap try and catch statements in
     #ifdef GLIBMM_EXCEPTIONS_ENABLED
     #endif //GLIBMM_EXCEPTIONS_ENABLED
   A line in configure.ac suggests that GLIBMM_EXCEPTIONS_ENABLED is always
   defined now and in the foreseeable future.
Comment 8 Kjell Ahlstedt 2011-02-14 15:36:38 UTC
Created attachment 180832 [details] [review]
patch: Add OptionGroup::add_entry() that takes a slot with callback function.

This patch implements both part A and part B of comment 7.

A. Added OptionGroup::add_entry() and add_entry_filename() that take a slot.
Added protected option_arg_callback().

B. An exception thrown by on_pre_parse() or on_post_parse() is propagated
to the error argument of g_callback_pre_parse() or post_parse_callback().
Comment 9 Murray Cumming 2011-02-15 11:38:28 UTC
>    Is it still necessary to wrap try and catch statements in
>      #ifdef GLIBMM_EXCEPTIONS_ENABLED
>      #endif //GLIBMM_EXCEPTIONS_ENABLED
>    A line in configure.ac suggests that GLIBMM_EXCEPTIONS_ENABLED is always
>    defined now and in the foreseeable future.

No, we stopped supporting that option a while ago.
Comment 10 Murray Cumming 2011-02-15 11:51:43 UTC
I have pushed this now. Many thanks.
Comment 11 Murray Cumming 2011-02-15 11:58:55 UTC
However, when using --enable-warnings=fatal, we get this warning, which is suprising, as it's a common technique. We need to avoid that warning before we can do a release.

optiongroup.cc: In member function ‘void Glib::OptionGroup::CppOptionEntry::allocate_c_arg()’:
optiongroup.cc:498: error: ISO C++ forbids casting between pointer-to-function and pointer-to-object
Comment 12 Kjell Ahlstedt 2011-02-15 16:38:46 UTC
I found some information at
http://www.trilithium.com/johan/2004/12/problem-with-dlsym/

It says there that many compilers accept conversion between a function pointer
and a data pointer if you use a C-style cast. g++ 4.4.5 does not.
An ugly union "solves" the problem. There is no warning from code like this:

  union {
    void* dp;
    GOptionArgFunc fp;
  } really_ugly_hack;

  really_ugly_hack.fp = &OptionGroup::option_arg_callback;
  carg_ = really_ugly_hack.dp;

The compiler accepts this code too:

  carg_ = reinterpret_cast<void*>(
          reinterpret_cast<unsigned long>(&OptionGroup::option_arg_callback));

I find it less ugly, but I wonder if it works correctly on all computers where
the union does. What if an unsigned long is longer than a pointer? Will the
cast to void* fetch the pointer from that part of the long where the other
cast has stored it? Probably, but can we be sure? And if long is replaced by
int, the question is: Can int be shorter than a pointer?

The real problem of course is that there is no guarantee that a function
pointer has the same size as a data pointer. A good solution requires a change
in glib. GOptionEntry::arg_data should not be used for storing either a data
pointer or a function pointer.

What do you think? Is any of these solutions acceptable?

glib uses a C-style cast in goption.c to convert from void* to a function
pointer. But that's not an option for us, it seems.
Comment 13 Murray Cumming 2011-02-16 10:58:42 UTC
Created attachment 180977 [details] [review]
glibmm_optiongroup_argcallback_warning.patch

I think I'd rather use a public friend function, just this once.

However, when I try that I still get the same compiler error, presumably because the compiler is parsing the friend declaration wrongly. Or do you see an error in my attached patch?
Comment 14 Chris Vine 2011-02-16 11:56:40 UTC
Comment #12: "The real problem of course is that there is no guarantee that a function pointer has the same size as a data pointer. A good solution requires a change in glib. GOptionEntry::arg_data should not be used for storing either a data pointer or a function pointer."

I don't think this is a problem with any glib wrapper, because glib itself casts between, for example, GClosure* and GCallback (see gclosure.c).  It is required to work in POSIX (dlsym() returns a function pointer by void* where the shared object is a function) and it also works with windows (apparently).  glib appears to rely on this.

You should avoid a compiler warning if you cast via address of void* and address of function pointer).  The canonical way of using dlsym() in the POSIX documentation is:

void    *handle;
int     (*fptr)(int);
handle = dlopen("/usr/home/me/libfoo.so", RTLD_LOCAL | RTLD_LAZY);
*(void **)(&fptr) = dlsym(handle, "my_function");

If your reinterpret_cast takes this approach it should be OK.
Comment 15 Kjell Ahlstedt 2011-02-16 13:34:36 UTC
(Reply to comment 13)
> However, when I try that I still get the same compiler error, presumably
> because the compiler is parsing the friend declaration wrongly. Or do you see
> an error in my attached patch?

It's still a function and not an object. Strict C++ rules do not allow a cast
from function pointer to data pointer.


I tested the code in comment 14 (approximately). The result is
  error: dereferencing type-punned pointer will break strict-aliasing rules

Translating the idea in comment 14 to our case I arrived at
*reinterpret_cast<GOptionArgFunc*>(&carg_) = &OptionGroup::option_arg_callback;

It also results in
optiongroup.cc:516: error: dereferencing type-punned pointer will break strict-aliasing rules

And I tried to build parts of glib with the compiler flags -pedantic -Werror.
I doesn't work! glibmm has stricter requirements on the code than glib has.
Comment 16 Krzesimir Nowak 2011-02-16 13:59:22 UTC
(In reply to comment #15)
> Translating the idea in comment 14 to our case I arrived at
> *reinterpret_cast<GOptionArgFunc*>(&carg_) = &OptionGroup::option_arg_callback;
> 
> It also results in
> optiongroup.cc:516: error: dereferencing type-punned pointer will break
> strict-aliasing rules

Union hack is common solution to this warning. It is often used when using posix socket stuff:

// find out address of eth0
struct ifreq		SInterfaceRequest; 
strcpy( SInterfaceRequest.ifr_name, "eth0" );
SInterfaceRequest.ifr_addr.sa_family = AF_INET;
if ( ioctl( iServer, SIOCGIFADDR, &SInterfaceRequest ) < 0 )
{
	cout << "ioctl() function failed." << endl;
	return false;
}
// TODO: use -fno-strict-aliasing or create a union containing sockaddr_in and sockaddr.
union
{
	struct sockaddr     sa;
	struct sockaddr_in  sa_in;
} u;
u.sa = SInterfaceRequest.ifr_addr;
char *pMyIp = inet_ntoa( u.sa_in.sin_addr );


And that C++ strictness on casting function pointer to object pointer or vice versa probably stems from specifics of harvard architecture where instructions and data have separate address spaces, hm?
Comment 17 Kjell Ahlstedt 2011-02-16 16:12:21 UTC
My conclusion is that if you want to build glibmm with --enable-warnings=fatal,
that is with the compiler flags -pedantic -Werror, and you want to wrap
GOptionEntry with G_OPTION_ARG_CALLBACK, then you must accept one of the
constructs in comment 12 or something equally ugly. (But if you choose the
union, I don't recommend the name of the union that I chose there.)

I suspect that no compiler can find a reason to reject a union. And as far as I
can see, it must give correct results on all computer architectures where all
pointers (function and data) have the same size. I don't like unions, but in
this case I feel forced to recommend one.

I've tested both solutions in comment 12 (union and double reinterpret_cast)
on the test case glibmm/examples/options. The results are correct on my PC
running Ubuntu 10.10.

Another conclusion is that I must start compiling my code with
--enable-warnings=fatal before I make patches.
Comment 18 Chris Vine 2011-02-16 17:37:53 UTC
Comment #15: "*reinterpret_cast<GOptionArgFunc*>(&carg_) = &OptionGroup::option_arg_callback;"

Shouldn't that be:

*reinterpret_cast<GOptionArgFunc*>(&carg_) = OptionGroup::option_arg_callback;

That may or may not stop the type-punning warning but it would be the correct version if you are going to cast via the address-of operator.  Putting in extra parentheses makes the meaning clearer:

*(reinterpret_cast<GOptionArgFunc*>(&carg_)) = OptionGroup::option_arg_callback;

Giving a type-punning warning on a reinterpret_cast is ridiculous anyway.  One of the main purposes of reinterpret_cast is to carry out type-punning.  Most other things are dealt with by static_cast.  It is however guaranteed to compile on any C compiler, whereas a direct cast from data pointer to function pointer is not (notwithstanding that glib actually does that in its own code - glib relies two extensions: the posix and windows extension which guarantees data pointer and function pointer have the same size and layout; and a compiler extension permitting the cast without going via the address-of operator).
Comment 19 Chris Vine 2011-02-16 18:19:40 UTC
Hmmm, and it looks as if the union hack will only work with gcc.  This is an interesting read:

http://mail-index.netbsd.org/tech-kern/2003/08/11/0001.html
Comment 20 Chris Vine 2011-02-16 19:15:16 UTC
"Hmmm, and it looks as if the union hack will only work with gcc".

On further thought I think that is wrong.  The POSIX standard, System Interfaces, section 2.12.3, Pointer Types implicitly requires a POSIX conforming compiler not to apply the strict aliasing rule to casts between void* and pointer to function. (In a similar way to ISO C requiring the strict aliasing rule not to apply to casts from any pointer type to pointer to character.)  Nothing explicit is said about unions, but it would be perverse for a union to break the rule if the explicit cast does not.
Comment 21 Kjell Ahlstedt 2011-02-16 20:12:36 UTC
(Reply to comment 18)
> Shouldn't that be:
> *reinterpret_cast<GOptionArgFunc*>(&carg_) = OptionGroup::option_arg_callback;

It doesn't matter, according to Bjarne Stroustrup's "The C++ Programming
Language", 3rd edition, section 7.7:
"Dereferencing of a pointer to function using * is optional. Similarly, using &
to get the address of a function is optional:
  void (*f1)(string) = &error; // ok
  void (*f2)(string) = error;  // also ok; same meaning as &error"

> That may or may not stop the type-punning warning but it would be the correct
> version if you are going to cast via the address-of operator.  Putting in extra
> parentheses makes the meaning clearer:
> *(reinterpret_cast<GOptionArgFunc*>(&carg_)) =
> OptionGroup::option_arg_callback;

Alas, it does not stop the type-punning warning.
In this case it's not the -pedantic flag but the optimization level (-O2) that
causes the warning. With -O1 this reinterpret_cast is accepted.

(Reply to comment 19)
> Hmmm, and it looks as if the union hack will only work with gcc.  This is an
> interesting read:
> http://mail-index.netbsd.org/tech-kern/2003/08/11/0001.html

Yes, that's interesting. If I interpret it correctly, another compiler than gcc
may get confused by a too cleverly used union when it optimizes the code. Or
perhaps report that one component of the union is uninitialized when only the
other component has been assigned a value.
I've seen comment 20, but I still wonder if it's really safe to use a union as
a replacement for a cast.

I suppose it's out of the question to change the compiler flags in glibmm.
Then of all the suggestions presented so far only the double reinterpret_cast
in comment 12 remains:

  carg_ = reinterpret_cast<void*>(
          reinterpret_cast<unsigned long>(OptionGroup::option_arg_callback));

I'm sure it's possible to do dirty tricks with a function with a variable-
length argument list, but that's a last resort.

  void* dirty_cast(int dummy, ...);
  carg_ = dirty_cast(0, OptionGroup::option_arg_callback);
Comment 22 Chris Vine 2011-02-16 21:01:16 UTC
"It doesn't matter, according to Bjarne Stroustrup's 'The C++ Programming Language', 3rd edition, section 7.7: 'Dereferencing of a pointer to function using * is optional. Similarly, using & to get the address of a function is optional.'"

Yes, but here you were assigning the address of the function pointer (OptionGroup::option_arg_callback), not the address of the function to which it refers, so there is a very big difference.

On the warning, I wonder if just using a C style cast still gives a warning, it really shouldn't do?

"I've seen comment 20, but I still wonder if it's really safe to use a union as
a replacement for a cast."

Yes I think it is and I think I was raising a red herring.  carg_ is never directly dereferenced.  When it is cast back to a function pointer in order to be accessed as an lvalue by dereferencing it, the function pointer type will be "a type compatible with the effective type of the object" which is also a function pointer not pointer to void.  In other words, with the POSIX extension (and similar extension for windows), passing a function pointer through a pointer to void and then back again cannot it seems to me be an error.

So if '*(OptionArgFunc*)&carg_ = OptionGroup::option_arg_callback;' gives a warning, I should go for the union.
Comment 23 Chris Vine 2011-02-16 21:19:41 UTC
"Yes, but here you were assigning the address of the function pointer (OptionGroup::option_arg_callback), not the address of the function to which it refers, so there is a very big difference."

I am wrong about this of course if OptionGroup::option_arg_callback is the name of a static member function rather than the name of a function pointer.  Apologies if I have got that wrong.
Comment 24 Kjell Ahlstedt 2011-02-17 10:31:25 UTC
Created attachment 181111 [details] [review]
patch: OptionGroup: Fix build error, remove memory leak.

The patch in comment 8 not only causes build errors. It also introduced a
memory leak, if the same OptionEntry is added twice to the same OptionGroup.
This patch fixes both problems. I've replaced the problematic cast with a
union. It remains to be seen if all compilers accept that solution.

Reply to comment 22 and comment 23:
'*(GOptionArgFunc*)&carg_ = OptionGroup::option_arg_callback;' gives a warning.
OptionGroup::option_arg_callback is the name of a static member function.
Comment 25 Chris Vine 2011-02-17 12:48:09 UTC
"I've replaced the problematic cast with a union. It remains to be seen if all compilers accept that solution. .... OptionGroup::option_arg_callback is the name of a static member function."

I don't think you need to be too picky about the union.  If gpointer carg_ is passed to glib for execution as a callback (that is, subsequently cast back to GOptionArgFunc for glib), a strict compiler is going to complain about that (assuming it can see it) long before it complains about the union, as static member functions have a C++ linkage specification not C linkage specification, and so might use a different calling convention (some intel compilers in fact do do so).  Although the C++ standard says these are different types, gcc will be fine: it uses the C calling convention for static member functions, and glibmm relies on that elsewhere (I think) so you aren't adding a new problem.

That is the reason I was a bit slow on the uptake about what the OptionGroup::option_arg_callback member comprised.  It may possibly also explain the strict aliasing warning, although I doubt it: the compiler might possibly be clever enough to know that casting a function with C++ linkage specification to void* is not guaranteed by posix and therefore technically breaks the strict aliasing rules.
Comment 26 Murray Cumming 2011-02-18 11:18:20 UTC
Would someone like to file a glib bug saying why the API is bad, as described here:
  http://library.gnome.org/devel/glib/unstable/glib-Commandline-option-parser.html#GOptionEntry
  "If the arg type is G_OPTION_ARG_CALLBACK, then [gpointer] arg_data must point to a GOptionArgFunc callback function"
They would be most responsive to a description of what bad things can happen if that is not improved.
Comment 27 Murray Cumming 2011-02-18 11:52:03 UTC
(In reply to comment #24)
> Created an attachment (id=181111) [details] [review]
> patch: OptionGroup: Fix build error, remove memory leak.

Thanks. I have pushed that. It will do at least for now. It keeps the hack fairly isolated.
Comment 28 Murray Cumming 2011-02-18 11:57:50 UTC
(In reply to comment #15)
> (Reply to comment 13)
> > However, when I try that I still get the same compiler error, presumably
> > because the compiler is parsing the friend declaration wrongly. Or do you see
> > an error in my attached patch?
> 
> It's still a function and not an object. Strict C++ rules do not allow a cast
> from function pointer to data pointer.

You are right. It's only the second line that causes the warning with this version:
  GOptionArgFunc cfunc = &OptionGroup_option_arg_callback;
  carg_ = reinterpret_cast<void*>(cfunc);
I guess I misunderstood the compiler error, thinking it was talking about pointers to member methods of objects:
  "error: ISO C++ forbids casting between pointer-to-function and pointer-to-object"
Comment 29 Kjell Ahlstedt 2011-02-20 18:07:44 UTC
(In reply to comment #26)
> Would someone like to file a glib bug saying why the API is bad

I have filed bug 642823 "Forbidden pointer conversion in GOptionEntry".
I have not been able to come up with a fix that preserves API/ABI. Unless
such a fix can be found, I suppose it will take a long time before the bug can
be fixed.

I have also filed bug 642825 "Unnecessary assertion failure in
g_option_context_parse()". It's related to bug 588989 rather than to this bug.
Comment 30 Murray Cumming 2011-02-21 10:02:22 UTC
Thanks very much. Then I think we are all done with this for glibmm.
Comment 31 Kjell Ahlstedt 2011-02-23 09:26:22 UTC
I compiled all of glib and gtk+ with the compiler flag -pedantic (but not
-Werror). It resulted in hundreds of warnings, many of which concerned
conversions between function pointers and object pointers. The glib and gtk+
developers don't strive for ISO C correctness. They are more pragmatic than
pedantic. See also bug 642823 comment 1.

Even when an API break will be allowed, the chances of a change in GOptionEntry
are so slim that I'm likely to close bug 642823 in a few days unless someone
objects.
Comment 32 Chris Vine 2011-02-23 15:40:54 UTC
I agree there is little chance of getting this changed in glib, but on the "unless someone objects" I don't think that any hypothetical "someone" should be too critical about glib not striving for ISO C correctness when glibmm does not itself strive for ISO C++ correctness.  In both cases, pragmatic decisions have been taken (or possibly the non-correctness was not spotted when the code was first written).

It just so happens that in -pedantic mode, g++ finds some non-ISO C constructs while missing some non-ISO C++ constructs (in particular as to language linkage).  Future versions of g++ may not be so accommodating.