GNOME Bugzilla – Bug 589197
no way to have an arg callback for a Glib::OptionEntry
Last modified: 2011-02-23 15:40:54 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.
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.
(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.
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.
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.
The reference to comment 1 in comment 4 is wrong. Shall be comment 3.
This is a bit spaghetti-like now. Could you just state the current situation in a self-contained way, please?
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.
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().
> 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.
I have pushed this now. Many thanks.
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
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.
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 #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.
(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.
(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?
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 #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).
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
"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.
(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);
"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.
"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.
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.
"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.
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.
(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.
(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"
(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.
Thanks very much. Then I think we are all done with this for glibmm.
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.
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.