GNOME Bugzilla – Bug 305331
Refactoring of main.c
Last modified: 2005-07-12 20:54:41 UTC
This is a patch which refactors parts of main.c to make the code more readable. I have made the following changes: 1. Moved the code for option parsing and printing of compilation info out of main() into three new functions. 2. Replaced the optionparsing code with new code that uses getopt. 3. Added a struct to contain the option arguments, display_name, client_id, etc, that previously were free variables. No features was changed except that getopt makes the option parsing slightly better.
Created attachment 46834 [details] [review] Patch to 305331
Comment on attachment 46834 [details] [review] Patch to 305331 Should use the GLib option parsing stuff rather than getopt; getopt_long is a GNU libc thing and not portable afaik, and we should do things the "Glib way" Indentation is a bit off in print_compilation_info Need spaces before parens in print_self_identity, and a (void) in the declaration Some indentation issues also in that function and in the MetaArguments struct and meta_parse_options I think the spirit of the patch is good, would like to see an updated version.
Created attachment 46954 [details] [review] Updated patch to parse options with glib OK! Here is the new version of the patch. The following are the changes from the previous patch: 1. Uses glib for option parsing. 2. Which deprecates the function usage() since glib's option parsing provides a much nicer one. 3. Added descriptions to the options. 4. A few new comments. 5. Tried to match the existing C-style in Metacity much more closely.
Comment on attachment 46954 [details] [review] Updated patch to parse options with glib MetaArguments should be passed by address: meta_parse_options (&argc, &argv, &meta_args); looks pretty good to me otherwise
Created attachment 47107 [details] [review] command line parsing using glib Ok. The only change from the previous change is that now the struct is returned in the parameter. Although I don't understand why you prefer it that way. Isn't it much nicer to pass values back with the return statement? I guess you know something I don't.
Comment on attachment 47107 [details] [review] command line parsing using glib Returning structs by value just isn't done in most C code, or at least in GLib/GTK code. It's a stylistic thing. I find it confusing/unexpected at least. I think this will fail in any compiler other than gcc, though maybe C99 allows it: &meta_args->disable_sm, I think you have to create a temporary MetaArguments, then assign it to *meta_args at the end of the function. I could be wrong. Patch looks fine to me with that change.
Created attachment 47652 [details] [review] Most updated glib parsing OK! Done I think. I hope this is how you mean. By the way, wouldn't it maybe be good to use c99? Then you could write for(int n = 0; ...) :)
Comment on attachment 47652 [details] [review] Most updated glib parsing Looks perfect, thank you
Björn: There's an awful lot of Gnome people requiring that c99 features not be used in general Gnome programs; something about portability to obsolete/brain-damaged systems. ;-)
Committed. 2005-07-12 Elijah Newren <newren@gmail.com> Patch from Björn Lindqvist to split up main() into more manageable chunks and make use of GOpt. Closes #305331. * src/main.c (usage): remove this function, (meta_print_compilation_info): new function taken from main(), (meta_print_self_identity): new function taken from main(), (struct MetaArguments) new struct to replace some free variables, (meta_parse_options): new funcion taken from main() but now using GOpt, (meta_select_display): new function taken from main()