Date: Fri, 13 Dec 2013 13:40:41 -0500 From: Pedro Giffuni <pfg@FreeBSD.org> To: Bruce Evans <brde@optusnet.com.au>, Sean Bruno <sbruno@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r258139 - head/contrib/gperf/src Message-ID: <52AB54A9.6040306@FreeBSD.org> In-Reply-To: <20131115145452.G954@besplex.bde.org> References: <201311141841.rAEIfwFU040620@svn.freebsd.org> <20131115145452.G954@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Hello; Sorry for the delay in looking into this issue. On 15.11.2013 00:06, Bruce Evans wrote: > On Thu, 14 Nov 2013, Sean Bruno wrote: > >> Log: >> Repair build after svn r258115 >> >> options.get_size_type() appears to return a const char *, so assume >> that >> its a string as oppose to *nothing*. I have no idea what apple's >> code is >> trying to do here: >> >> http://opensource.apple.com/source/gperf/gperf-9/patches/size_type.patch >> > > It is trying to add an arg to a printf, while retaining the old behaviour > of printing nothing (now including the new arg) in some cases. The > compiler doesn't like this, since the new arg is unconditional while > the format is conditional. I think it is a compiler bug to warn in > this case. > > This commit replaces the warning (which is really about the compiler bug) > by a bug in gperf. But I think the bug is harmless because it is in > unreachable code. But then why have the unreachable code? (It is for > a default case which I think is unreacheable because all possible cases > that occur are handled individually.) > I decided not to merge that patch from Apple, after all it has limited (no) use at this time. For the code in current, I agree the warning is bogus and I think it would be better to revert Sean's workaround and set the WARNS level to zero. This code is unlikely to evolve further before it is removed from the tree and we won't really need extra warnings for it. Does that sound acceptable? The alternative would be, of course, just reverting the Apple patch. Regards, Pedro. >> Modified: head/contrib/gperf/src/output.cc >> ============================================================================== >> >> --- head/contrib/gperf/src/output.cc Thu Nov 14 16:10:21 2013 >> (r258138) >> +++ head/contrib/gperf/src/output.cc Thu Nov 14 18:41:58 2013 >> (r258139) >> @@ -779,7 +779,7 @@ Output::output_hash_function () const >> " register %s len;\n" : >> option[ANSIC] | option[CPLUSPLUS] ? >> "(register const char *str, register %s len)\n" : >> - "", option.get_size_type()); >> + "%s", option.get_size_type()); >> >> /* Note that when the hash function is called, it has already been >> verified >> that min_key_len <= len <= max_key_len. */ >> ... > > The old code hard-coded the type of the integer arg as "unsigned > int". This > is now spelled "%s" with a new arg. When there is no declaration at all, > then there is no integer arg and the declaration was spelled "". This is > still the correct spelling. Changing it to "%s" gives a syntax error > like > a bare "unsigned int" in the output. However, this case seems to be > unreachable. > > The printf() is rather complicated and fancily formatted, with the fancy > format partly destroyed by the svn mail bug of not quoting patches and > mail > programs then sometimes removing leading whitespace. It uses a nice > conditional ladder of the following form: > > printf( > case1 ? "decl1 %s len" : > case2 ? "decl2 %s len" : > case3 ? "decl3 %s len" : > /* default (unreachable?) */ "", typestr); > > where the formatting is much fancier than this (but is perfectly > consistent > except for the default case and now for the option at the end (the option > should be on a line by itself and not grouped with the complicated > conditional ladder, especially not with its unreachable part). > > The cases are: > > case1: K&R C > case2: plain C > case3: ANSI (sic) C or C++ > default: unreachable? > > and the generated code is now: > > K&R C: "register <type> len'\n" > plain C: "register <type> len;\n" > ANSI (sic) C or C++: "register <type> len;\n" > unreachable?: "<type>" /* syntax error */ > > Before this commit, the generated code was: > > K&R C: "register <type> len'\n" > plain C: "register <type> len;\n" > ANSI (sic) C or C++: "register <type> len;\n" > unreachable?: "" /* no syntax error */ > > and a few days ago it was: > > K&R C: "register unsigned len'\n" > plain C: "register unsigned len;\n" > ANSI (sic) C or C++: "register unsigned len;\n" > unreachable?: "" /* no syntax error */ > > This particular declaration doesn't even depend on the language. The > printf() just groups it with another one for a pointer because this used > to be convenient. > > The supported languages are sort of documented in the man page. ANSI C > doesn't exist, and there have been several versions of Standard C since > it existed, but there is no special version-dependent support. It is > even less clear what plain C is. It seems to be for a 1980's C that > is not quite as old as K&R C -- according to the generated code, plain C > has const but not signed char, while K&R C has neither. I get the lack > of signed char from smallest_integral_type(). smallest_integral_type() > is fundamentally wrong since it uses the host SCHAR_MIN for building > K&R and plain C targets that don't have SCHAR_MIN and might even have > a different character size when running on the same hardware as gperf. > > Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?52AB54A9.6040306>