Andy Koppe wrote: > Attached is a largish patch for adding locale/charset support to > mkshortcut and fixing a couple of bugs. It also contains quite a bit > of code cleanup.
Thanks for your efforts, Andy. I really appreciate it. > ChangeLog: > * src/mkshortcut/mkshortcut.c: > - Add locale/charset support and remove Win9x-specific code. > - Use Unicode versions of Windows function. > - Remove potential buffer overflows due to unguarded uses of strcpy. > - Factor out error-reporting boilerplate. OK. Except that this "factoring out" is also part of the "pointless freeing" below. > - Remove unnecessary function arguments and pointless freeing. I disagree with most of these changes. Also, unless you plan to update the style of all of the OTHER popt-using utilities in cygutils, I don't think it is wise to change mkshortcut in this way. Personally, I dislike global static variables. It's also bad practice with popt, because a single program can have multiple parsing contexts. (Not that any of the cygutils tools do this; so if you want to globally modify all cygutils popt-using utilities to make optCon a static var, I would accept that patch; but let's do it separately -- but, like "free" discussion below, we need to preserve the call to poptFreeContext()). I think that, in general, --help output should go to stdout, not stderr. Also --usage, --license, and --version output. However, if an *error* is handled by printing the usage(), then it should go to stderr. That wasn't the case in mkshortcut. So, rather than removing the FILE * argument -- since, as you noticed, all of the affected functions were invariably called with stderr -- I'd prefer that the ?,u,v,l cases be updated to call the appropriate function with stdout, but that existing error handling cases call usage() with stderr. (Ick. It appears that most of the other popt-using cygutils tools ALSO send everything always to stderr. Well, this is an easy change. Let's punt on this, too, and I'll do it all-at-once) Finally, what you call "pointless freeing" I call "good coding practice". Sure, in each case the very next thing that will happen is exit() or abort(), so the system will reclaim that memory immediately. So these "leaks" are harmless -- but they ARE leaks, and valgrind/duma/purify will report them as such. If you want to consolidate error handling, which is probably a good idea, then a better approach is to register each malloc'ed pointer with a cleanup function [*] and always call that before exit(). [*] or put them all in a static struct or two [**] (sigh), and use the XFREE() macro on each member (from a simple cleanup function that you always call before exit; e.g. from within nullcheck() and error(), as well as the explicit exit() calls). Or create individual cleanup functions for each one, and register them with atexit(). Or combine these two approaches and register a single cleanup function with atexit(). Or clean them up "locally", as the existing code did. I realize the problem is harder in your version, because many of the static buf[] variables are now dynamically allocated, but I still think it's good practice to clean up own messes, even just before calling exit(). [**] I'm thinking one struct for the vars malloc'ed in main(), and another for those in mkshortcut(). > - Return zero for success from mkshortcut(). Derr...oops. Good catch. -- Chuck
