[flashrom] [PATCH] Fix compilation with only one enabled programmer.

[Stefan Tauner]

On Sat, 21 Nov 2015 09:29:33 +0100
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:

> On 21.11.2015 00:43, Stefan Tauner wrote:
> > Compilers are free to use an unsigned data type for enumerations without
> > negative elements. Our enumeration of all programmers is one such
> > instance. Also, compilers will warn about comparisons of unsigned enum
> > expressions < 0.
> >
> > If we enable only a single programmer then PROGRAMMER_INVALID will be 1.
> > We do some run-time checks of loop counters of an unsigned enumaration
> > type against PROGRAMMER_INVALID in the form of
> >   if (p < PROGRAMMER_INVALID - 1)
> > which get optimized to
> >   if (p < 0)
> > This makes the check always false and the compiler warnings compulsory.
> 
> Which gcc version is this?

clang :) 3.0
At least my gcc 4.6.3 does not bother to warn and/or make the enum
unsigned - at least not with our default CFLAGS.

> > This patch adds #ifdefs guarding these checks if there is only one programmer
> > enabled.
> >
> > ---
> > One can enable a single programmer by running
> > grep CONFIG_ Makefile |fgrep '?= yes'|grep -vi serprog |sed "s/ .*/=no/"|xargs make  CONFIG_SERPROG=yes
> >
> > Signed-off-by: Stefan Tauner <stefan.tauner at alumni.tuwien.ac.at>
> > ---
> >  flashrom.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/flashrom.c b/flashrom.c
> > index c9c7e31..abb917b 100644
> > --- a/flashrom.c
> > +++ b/flashrom.c
> > @@ -1633,10 +1633,13 @@ void list_programmers(const char *delim)
> >  	enum programmer p;
> >  	for (p = 0; p < PROGRAMMER_INVALID; p++) {
> >  		msg_ginfo("%s", programmer_table[p].name);
> > +
> > +#if PROGRAMMER_INVALID > 1
> >  		if (p < PROGRAMMER_INVALID - 1)
> 
> Does the warning also disappear if you write
> if ((PROGRAMMER_INVALID > 1) && (p < PROGRAMMER_INVALID - 1) )
> instead? AFAICS that should placate gcc (since this is a constant false
> expression for PROGRAMMER_INVALID==1) even with warnings switched on.

That's the first thing I tried, sadly without success.

> >  			msg_ginfo("%s", delim);
> > +#endif
> >  	}
> > -	msg_ginfo("\n");	
> > +	msg_ginfo("\n");
> >  }
> >  
> >  void list_programmers_linebreak(int startcol, int cols, int paren)
> > @@ -1668,6 +1671,7 @@ void list_programmers_linebreak(int startcol, int cols, int paren)
> >  		}
> >  		msg_ginfo("%s", pname);
> >  		remaining -= pnamelen;
> > +#if PROGRAMMER_INVALID > 1
> >  		if (p < PROGRAMMER_INVALID - 1) {
> >  			msg_ginfo(",");
> >  			remaining--;
> > @@ -1675,6 +1679,9 @@ void list_programmers_linebreak(int startcol, int cols, int paren)
> >  			if (paren)
> >  				msg_ginfo(")");
> >  		}
> > +#else
> > +		msg_ginfo(")");
> 
> Please drop the complete #else block. It adds a ")" where none should
> be. It doesn't just look strange in the code, I also verified the output.

Will re-check that, thanks.

Now the question is, do we want that? or do we want to hack around
that? E.g. with one negative value in the enumeration or casts?

-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner