[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Bug-gnubg] Patch: File associations under Windows and spaces in fil
From: |
Jim Segrave |
Subject: |
Re: [Bug-gnubg] Patch: File associations under Windows and spaces in filenames |
Date: |
Mon, 25 Nov 2002 13:17:10 +0100 |
User-agent: |
Mutt/1.4i |
On Mon 25 Nov 2002 (01:37 +0100), Holger wrote:
> Hi,
>
> The attached patch makes file associations under Windows (double-clicking
> on a match) work and provides the ability to load matches with whitespace
> characters in the path or filename.
>
> Since it's my first source code contribution and I certainly don't have the
> programming experience and insight to the GNUbg source like you developers
> I'd like to ask you to check and test the patch thoroughly. Of course I've
> tried not to break anything, but you never know.
>
> The patch is against the 021121 snapshot.
A few suggestions, just from looking at the diffs (please don't take
these as criticisms, there are things here that everyone should keep
in mind and, at least in some cases, I've failed to follow my own
advice):
Using diff:
1) make a copy of the original (before editing) file - Unix users
would usually use gnubg.c.orig, I don't know if Windows allows
multiple full stops in file names.
2) Diff output should be the result of (for example):
diff -b -w -u gnubg.c.orig gnubg.c > gnubg.c.diff
the -b option tells diff not to generate output for changes which
are only blank lines (usually these are irrelevant and simply
clutter up the patch).
the -w option tells diff not to generate output for changes which
are only changes in white space (if your editor changes spaces to
tabs or vice-versa for example). Again, usually these are
irrelevant).
the -u option is the most important - unified diffs are generally
easier to look at to see what's changed than the old context style
diffs (my opinion, but I think it's widely shared).
In the case of the diffs you submitted, running them against the
current CVS put the new local variables szQuoted et al into the
middle of the usage text, as there was no context from diff.
3) this may or may not meet with agreement from others, and I've
screwed up slightly before in doing this. In order to make it very
clear which version of a routine you have edited (021121 snapshot
isn't necessarily specific enough), you can do the following:
In your edited version, find the CVS $ID line and remove everything
except $Id$. Now the diff output will contain the contents of the
$Id$ line from the original as part of the changes, so there's no
question of where it came from. When it's checked into CVS, the ID
line will be regenerated with the new version no.
Of course doing something wrong, as I did recently and putting /*
$Id $ (note the extra space causes CVS not to put Id's in at all,
but in general, I think this is helpful. See the new version of
your diffs below - the first change tells what version of gnubg.c
the diff is agains.
Other comments:
1) When you added the --datadir option to the getopt_long call, you
missed the comments above, which should be changed to reflect the
change to the code. It's a mistake every programmer makes, but it
leads to a support nightmare. There's a saying somewhere
(Programming Pearls II perhaps) to the effect:
"If the comments and the code disagree, it is likely that both are
wrong."
2) I don't have a Windows environment, so I don't know if it supplies
PATH_MAX, but Unix environments don't supply _MAX_PATH. I added a
conditional directive to work around this:
#ifdef WIN32
#define BIG_PATH _MAX_PATH
#else
#define BIG_PATH PATH_MAX
#endif
and changed the declarations to use BIG_PATH. If the Windows
environment supplies PATH_MAX, then we should use that instead of this
workaround.
3) Again, this is my opinion only:
I would strongly recommend against using C++ style comments ('//
text') and only use /*..*/ ones. We're much better off staying with
ANSI C. The option is going to C++, but I don't think that that's
desireable/possible. I notice other developers who are also doing
this, but this really limits any build to being done with gcc or a
similarly tolerant compiler. The person trying to build a text only
version on an obscure platform (or with a compiler optimised for
his hardware) will thank us all for not expecting the compiler to
accept this.
Attached is an updated version of the diffs, I have committed this,
along with a fix to set.c (unrelated problem - under FreeBSD,
including sys/resource.h requires including sys/time.h, and changes to
Makefile.am - makebearoff1 needs to explicitly link with getopt and
getopt1, makebearoff.c had an extraneous single quote, causing a core dump)
--
Jim Segrave address@hidden
diffs.out.gz
Description: application/gunzip