Re: [PATCH 1/2] Fix segfault with 'Game List Tags' dialogue

Thomas Adam schreef op 3/21/2015 om 12:06 AM:
>
> I see---it seems a little... hmm, odd, that the presense of the dialogue
> is the thing to check.  That there is no data in glc seems the more
> technically correct thing to check for.
>
I think the two are always correlated, as XBoard always created the 
dialog as soon
as it gets a non-empty game list. I guess at the time it felt natural to 
test for this,
because I assumed that the actual segfault was due to accessing a 
non-existent widget.
So I added a test for its existence, I could have been wrong about that, 
though.

> It bothers me, because it's crufty, and often that looks bad, especially
> in terms of code structure, etc.
Well, I am not saying that this should not be done. I just explained I 
am not very motivated
to do it. I am basically only interested in adding new functionality, 
and then usually because
it is functionality I need myself as an XBoard/WinBoard user. So it is 
all the better if there
are other people that would be interested in cleaning up the code. 
(Which was already in
a pretty sorry state by the time I got involved.)

> I'm starting this already [1], there's a few topic branches where I've
> been able to make a start.  It's actually rather nice doing this because
> it's already flagged up a lot of interesting points which are on my
> radar, for instance these things (in no particular order):
>
> * if (f) free(f) -- free(NULL) is guaranteed to be a no-op;
> * free() versus FREE() versus free(f); f = NULL;
> * [cm]alloc() -> snprintf() -- just use asprintf();
> * strcpy / etc.
The FREE and ASSIGN macros are things I added myself relatively 
recently, because I got
a bit tired from writing "if(f) free(f); f=dup(x);" all the time. So in 
new code I use those.
But all the old code still writes it explicitly; I did not see any point 
in replacing working code,
as thie macros were intended to save me work, not produce it.
   It would be good to also have a macro DEBUG for the construct
"if(appData.debugMode") fprintf(debugFP, ", which is also something I 
have to type
annoyingly often.

I wasn't even aware something like asprintf existed. I don't know if 
there is a particular
reason avoiding it. (Like compatibility with old libraries that do not 
have it. Amazingly
enough even snprintf seems to give problems in MicroSoft Visual Studio.) 
In code I add
I usually print to declare static or automatic arrays, rather than to 
allocated memory.

At one point we tried to replace all strcpy by safeStrCpy. But being 
dogmatic about that
often leads to very silly code, like safeStrCpy(*command, 512, "go\n"); 
where there is
no doubt at all that the 3 printed characters will fit in the target, 
and you have to assume
the size of the target anyway, because it is only available as (char*). 
Things like
safeStrCpy(*command, strlen("go\n"), "go\n") are even worse. So I still 
use strcpy
sometimes for cases where there is no doubt at all that there cannot be 
any buffer overflow.

> That was a question I had:  I do not have access to a windows PC.  How
> do you ensure changes made doesn't break Winboard?  Indeed, whilst I
> might *just* about be able to cross-compile it under Linux, I certainly
> couldn't check the run-time results.  So any thing I do would only be
> with respect to XBoard itself.
Well, this is a problem. Anything change in the back-end that would 
require a compensating
change in the XBoard front-end are likely to break WinBoard, if the 
WinBoard front-end
would not get the compensating change too. A weaker problem is 
functional equivalence;
it is undesirable if features added to XBoard are not operational in 
WinBoard for lack of
front-end functionality, even though they compile OK.