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

On Mon, Oct 21, 2024 at 09:25:18AM +0200, Theo Buehler wrote:
> 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.

Got it, makes sense.

Thanks,
Nir