Re: [PATCH] Add locale/charset support to mkshortcut

[Andy Koppe <andy dot koppe at gmail dot com>]

2010/1/5 Charles Wilson:
> Personally, I dislike global static variables.

There may be good reasons to avoid global variables that are shared
between different source files, and they should certainly be treated
with utmost suspicion in multithreaded code. But in a small
single-threaded one-file program, I think this is superstition that
does nothing but clutter up the program and introduce opportunity for
errors. Passing around stderr is particularly silly, as that won't
make the global variable go away.


> (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

Fair enough. Then again, I'd be ok to accept this as a matter of
opinion anyway and add the (un)necessary arguments to the newly
introduced helper functions.


> 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. [...] Let's punt on this, too, and
I'll do it all-at-once.

Makes sense, and I'm certainly happy to leave that to you.


> 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.

I'm shocked you're serious about this, and couldn't disagree more.
These cases are not leaks in any meaningful sense of the word, since
as you say, the memory is released immediately afterwards anyway. This
is a valid technique for programs without significant lifetime and
memory usage. The "leak" detection in valgrind&co. can presumably be
switched off.

Trying to avoid these so-called leaks significantly complicates what
should be a very simple program by requiring every error to be
threaded back to the end of the main function. This is error-prone,
obscures the important bits of the program, and hinders development.
And it's completely pointless.


> Â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().
>
> [...]
>
> 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().

Your package, your rules. But my spare time, and I'm not prepared to
spend it on this. Oh, and my fault for not asking before making these
changes.

Feel free to pick the useful bits of code out of the patch. Obviously
I'm happy for it to be licensed under GPL2 (or later). Copyright
statement not required.

Andy