From: Theo Buehler <tb@theobuehler.org>
Subject: Re: Minor fixes in Fortune game
To: Nir Lichtman <nir@lichtman.org>
Cc: tech@openbsd.org
Date: Mon, 21 Oct 2024 09:25:18 +0200

On Mon, Oct 21, 2024 at 04:03:26AM +0000, Nir Lichtman wrote:
> On Sun, Oct 20, 2024 at 11:08:32PM +0200, Theo Buehler wrote:
> > It would be nice to have some explanations for the fixes. Maybe it's
> > just grumpy old me, but I find it unpleasant to be faced with a naked
> > diff, especially if it mixes a few not entirely obvious things that
> > have nothing to do with each other.
> > 
> 
> Sorry about that, noted for next times, I should have split to separate patches
> instead of putting all of these over here.
> 
> Adding comments below to explain.
> 
> > I took the leak fix. Comments inline
> > 
> > On Sat, Oct 05, 2024 at 09:46:10AM +0000, Nir Lichtman wrote:
> > > diff --git games/fortune/fortune/fortune.c games/fortune/fortune/fortune.c
> > > index 652422839..9a84b23cd 100644
> > > --- games/fortune/fortune/fortune.c
> > > +++ games/fortune/fortune/fortune.c
> > > @@ -163,8 +163,11 @@ main(int ac, char *av[])
> > >  
> > >  	init_prob();
> > >  	if ((Short_only && minlen_in_list(File_list) > SLEN) ||
> > > -	    (Long_only && maxlen_in_list(File_list) <= SLEN))
> > > +	    (Long_only && maxlen_in_list(File_list) <= SLEN)) {
> > > +		(void)fprintf(stderr,
> > > +			"no fortunes matching length constraint found\n");
> > 
> > Not sure why it should print something. Care to explain?
> 
> The idea behind this is to be more indicative in the case a custom fortune list
> is chosen in which all the fortunes are too short/too long, in the current
> situtation it would just exit with a generic failure, which can be confusing for
> the end user, there are more exits like this in the code, and optimally 
> all would print something, but started with this.
> The code already contains some paths which also print an error, besides exitting.

Alright thanks, committed without the void cast.

> > 
> > >  		return 1;
> > > +	}
> > >  
> > >  	do {
> > >  		get_fort();
> > > @@ -251,12 +254,10 @@ fortlen(void)
> > >  void
> > >  getargs(int argc, char *argv[])
> > >  {
> > > -	int	ignore_case;
> > > +	bool	ignore_case = false;
> > 
> > This is purely stylistic. Many people dislike bool (not sure why but
> > they do).
> 
> The code here already contains a couple of booleans so thought this would fit in
> to be more consistent

I know. I still think it's mostly a matter of taste and feels like a
step in the wrong direction. People have worked towards removing
booleans throughout the tree.