Re: [PATCH 06/11] build: include <config.h> from lib/yyerror.c

[Jim Meyering]

Akim Demaille wrote:

> Le 23 janv. 2012 à 11:57, Jim Meyering a écrit :
>
>> Hi Akim,
>>
>> How about this?
>
> Actually, for mhistorical reason, yyerror is really expected
> to return an int (by default, which is the case here).  I guess
> it dates back to the good ol' time o' C when not declaring
> was fine, and meant int.
>
> http://pubs.opengroup.org/onlinepubs/7908799/xcu/yacc.html
>
>> The following functions appear only in the yacc library accessible
>> through the -l y operand to cc or c89; they can therefore be
>> redefined by a portable application:
>> int main(void)
>> This function will call yyparse() and exit with an unspecified
>> value. Other actions within this function are unspecified.
>> int yyerror(const char *s
>> This function will write the NUL-terminated argument to standard
>> error, followed by a newline character.
>
>
>
>
>> I didn't find anyone using the return value, so rather than
>> trying to preserve semantics for nonexistent callers, I opted
>> to make this yyerror function return void as documented.
>
> What do you mean by "as documented"?

The one the manual suggest we provide in examples, e.g.,

    Let's consider an example, vastly simplified from a C++ grammar.

         %{
           #include <stdio.h>
           #define YYSTYPE char const *
           int yylex (void);
           void yyerror (char const *);
         %}


>> Oh, wait!
>> I do see one non-void use.  It's in data/glr.c:
>>
>>  return yyerror (]b4_yyerror_args[YY_("syntax error: cannot back up")),     \
>>
>> From the context, I'm not sure if that code is ever used:
>>
>>    ------------------------------------------
>>    # undef YYBACKUP
>>    # define YYBACKUP(Token, Value)                                           
>>    \
>>      return yyerror (]b4_yyerror_args[YY_("syntax error: cannot back up")),  
>>    \
>>             yyerrok, yyerr
>
> I don't see what you mean here: the return value is ignored by ','.

Good!  I was not reading carefully.

>> Anyhow, with that I'm not so sure it's ok to s/int/void/.
>
> I don't think it is.  But there is no semantics attached to that
> int.  0 would do fine.
>
>> * lib/yyerror.c (yyerror): Use fputs and fputc rather than fprintf
>> with a mere "%s\n" format.  Also, change the return type to void.
>> This avoids a problem reported by Thiru Ramakrishnan in
>> http://lists.gnu.org/archive/html/help-bison/2011-11/msg00000.html
>> ---
>> lib/yyerror.c |    7 ++++---
>> 1 files changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/yyerror.c b/lib/yyerror.c
>> index 5eb339f..5ac0438 100644
>> --- a/lib/yyerror.c
>> +++ b/lib/yyerror.c
>> @@ -20,10 +20,11 @@
>> #include <config.h>
>> #include <stdio.h>
>>
>> -int yyerror (char const *);
>> +void yyerror (char const *);
>>
>> -int
>> +void
>> yyerror (char const *message)
>> {
>> -  return fprintf (stderr, "%s\n", message);
>> +  fputs (message, stderr);
>> +  fputc ('\n', stderr);
>> }
>
> So you are engaging yourself gnulib will never #define these guys? :)
> Actually, is main.c's setlocale's also protected from such dependencies?

I now think it's better not to include <config.h> there, after all.
fputs and fputc are far less likely to require replacement.

Re setlocale, let's defer crossing that bridge until we come to it.
I don't expect gnulib will replace it any time soon.

>> #include <config.h>
>>
>> #if HAVE_LOCALE_H
>> # include <locale.h>
>> #endif
>> #if ! HAVE_SETLOCALE
>> # define setlocale(Category, Locale)
>> #endif
>>
>> int yyparse (void);
>>
>> int
>> main (void)
>> {
>>   setlocale (LC_ALL, "");
>>   return yyparse ();
>> }

Thanks for the reality check.
How about this revised patch?

>From 507aafd6b2388780aad6dfd349c65075c2b378e6 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Mon, 23 Jan 2012 11:47:46 +0100
Subject: [PATCH] build: avoid possibly-replaced fprintf in liby-source,
 yyerror.c

* lib/yyerror.c (yyerror): Use fputs and fputc rather than fprintf
with a mere "%s\n" format.  Always return 0 now, on the assumption
that the return value was never used anyway.
Don't include <config.h> after all.  This avoids a problem
reported by Thiru Ramakrishnan in
http://lists.gnu.org/archive/html/help-bison/2011-11/msg00000.html
* cfg.mk: Exempt lib/yyerror.c from the sc_require_config_h_first test.
---
 cfg.mk        |    3 ++-
 lib/yyerror.c |    5 +++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/cfg.mk b/cfg.mk
index 2d4f1ba..6b3deb9 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -55,4 +55,5 @@ update-copyright-env = \
   UPDATE_COPYRIGHT_FORCE=1 UPDATE_COPYRIGHT_USE_INTERVALS=1

 exclude_file_name_regexp--sc_space_tab = ^tests/(input|c\+\+)\.at$$
-exclude_file_name_regexp--sc_require_config_h_first = ^data/(glr|yacc)\.c$$
+exclude_file_name_regexp--sc_require_config_h_first = \
+  ^(lib/yyerror|data/(glr|yacc))\.c$$
diff --git a/lib/yyerror.c b/lib/yyerror.c
index 5eb339f..c9f492f 100644
--- a/lib/yyerror.c
+++ b/lib/yyerror.c
@@ -17,7 +17,6 @@
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */

-#include <config.h>
 #include <stdio.h>

 int yyerror (char const *);
@@ -25,5 +24,7 @@ int yyerror (char const *);
 int
 yyerror (char const *message)
 {
-  return fprintf (stderr, "%s\n", message);
+  fputs (message, stderr);
+  fputc ('\n', stderr);
+  return 0;
 }
--
1.7.9.rc2.2.g183d6