On Sat, 21 Nov 2015 09:29:33 +0100 Carl-Daniel Hailfinger <[email protected]> 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 <[email protected]> > > --- > > 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 _______________________________________________ flashrom mailing list [email protected] http://www.flashrom.org/mailman/listinfo/flashrom
