Re: [RFA] Fix glr.c's YYDEBUG and YY_ERROR_VERBOSE vs. glr.cc

[Paul Eggert]

Paolo Bonzini <address@hidden> writes:

> Thanks, here's the dozen lines that fix YYDEBUG, without the quite
> obvious bug you detected.  I'm using a member to return a meaningful
> value from get_debug_level.

OK, but wouldn't it be better to omit the debugging code entirely if
YYDEBUG is not set?  That's the tradition elsewhere.  I'm not a C++
expert, so I don't know how such things are usually done there, but
the current C++ code sometimes uses an #if and sometimes not, which
seems inconsistent.

I installed the following patch, which changes it to always use #if.

2006-09-13  Paul Eggert  <address@hidden>

        Fix glr.cc and lalr1.cc's use of YYDEBUG so that there's zero
        runtime cost when YYDEBUG is not defined, and so that some tests
        that used to fail now work.  Problem and initial suggestion by
        Paolo Bonzini.
        * data/c++.m4 (b4_parse_param_cons): Omit leading ','.
        * data/glr.cc (b4_parser_class_name):
        Initialize yycdebug_ only if YYDEBUG.  Also, initialize yydebug_.
        (debug_level, set_debug_level): Affect yydebug_, not ::yydebug.
        (yydebug_) [YYDEBUG]: New member.
        (yycdebug_): Now defined only if YYDEBUG.
        * data/lalr1.cc (yydebug_, yycdebug_): Now defined only if YYDEBUG.
        (YYCDEBUG) [!YYDEBUG]: Don't use yydebug_ and yycdebug_.
        (b4_parser_class_name): Initialize yydebug_ and yycdebug_ only
        if YYYDEBUG.
        (debug_stream, set_debug_stream, debug_level, set_debug_level):
        Define only if YYDEBUG.
        * tests/calc.at (_AT_DATA_CALC_Y) [!YYDEBUG]: Omit call to
        set_debug_level.
        * tests/regression.at (_AT_DATA_DANCER_Y) [!YYDEBUG]: Likewise.
        * tests/calc.at (AT_CHECK_CALC_GLR_CC): Uncomment calls to
        AT_CHECK_CALC_GLR_CC that are working now.

--- data/c++.m4 22 Jun 2006 19:46:05 -0000      1.8
+++ data/c++.m4 13 Sep 2006 17:39:53 -0000
@@ -101,7 +101,7 @@ m4_define([b4_parse_param_decl_1],
 # Extra initialisations of the constructor.
 m4_define([b4_parse_param_cons],
           [m4_ifset([b4_parse_param],
-                   [,
+                   [
       b4_cc_constructor_calls(b4_parse_param)])])
 m4_define([b4_cc_constructor_calls],
          [m4_map_sep([b4_cc_constructor_call], [,
--- data/glr.cc 12 Sep 2006 17:46:34 -0000      1.26
+++ data/glr.cc 13 Sep 2006 17:39:53 -0000
@@ -122,8 +122,12 @@ namespace ]b4_namespace[
 ]dnl In this section, the parse param are the original parse_params.
 m4_pushdef([b4_parse_param], m4_defn([b4_parse_param_orig]))dnl
 [  /// Build a parser object.
-  ]b4_parser_class_name::b4_parser_class_name[ (]b4_parse_param_decl[)
-    : yycdebug_ (&std::cerr)]b4_parse_param_cons[
+  ]b4_parser_class_name::b4_parser_class_name[ 
(]b4_parse_param_decl[)]m4_ifset([b4_parse_param], [
+    :])[
+#if YYDEBUG
+    ]m4_ifset([b4_parse_param], [  ], [ :])[yydebug_ (false),
+      yycdebug_ (&std::cerr)]m4_ifset([b4_parse_param], [,])[
+#endif]b4_parse_param_cons[
   {
   }
 
@@ -185,16 +189,16 @@ m4_pushdef([b4_parse_param], m4_defn([b4
   ]b4_parser_class_name[::debug_level_type
   ]b4_parser_class_name[::debug_level () const
   {
-    return ::yydebug;
+    return yydebug_;
   }
 
   void
   ]b4_parser_class_name[::set_debug_level (debug_level_type l)
   {
-    ::yydebug = l;
+    yydebug_ = l;
   }
 
-#endif /* ! YYDEBUG */
+#endif
 ]m4_popdef([b4_parse_param])dnl
 [} // namespace ]b4_namespace[
 
@@ -337,7 +341,10 @@ b4_user_stype
                                   const semantic_type* yyvaluep,
                                   const location_type* yylocationp);
   private:
-#endif /* ! YYDEBUG */
+    /* Debugging.  */
+    int yydebug_;
+    std::ostream* yycdebug_;
+#endif
 
 
     /// \brief Reclaim the memory associated to a symbol.
@@ -350,8 +357,6 @@ b4_user_stype
                             semantic_type* yyvaluep,
                             location_type* yylocationp);
 
-    /* Debugging.  */
-    std::ostream* yycdebug_;
 ]b4_parse_param_vars[
   };
 
--- data/lalr1.cc       11 Sep 2006 18:56:58 -0000      1.142
+++ data/lalr1.cc       13 Sep 2006 17:39:53 -0000
@@ -172,7 +172,7 @@ b4_error_verbose_if([, int tok])[);
     virtual void yy_symbol_print_ (int yytype,
                                   const semantic_type* yyvaluep,
                                   const location_type* yylocationp);
-#endif /* ! YYDEBUG */
+#endif
 
 
     /// State numbers.
@@ -249,6 +249,10 @@ b4_error_verbose_if([, int tok])[);
     virtual void yy_reduce_print_ (int r);
     /// Print the state stack on the debug stream.
     virtual void yystack_print_ ();
+
+    /* Debugging.  */
+    int yydebug_;
+    std::ostream* yycdebug_;
 #endif
 
     /// Convert a scanner token number \a t to a symbol number.
@@ -279,11 +283,6 @@ b4_error_verbose_if([, int tok])[);
     static const int yyntokens_;
     static const unsigned int yyuser_token_number_max_;
     static const token_number_type yyundef_token_;
-
-    /* Debugging.  */
-    int yydebug_;
-    std::ostream* yycdebug_;
-
 ]b4_parse_param_vars[
   };
 }
@@ -334,12 +333,12 @@ b4_defines_if([
 /* Suppress unused-variable warnings by "using" E.  */
 #define YYUSE(e) ((void) (e))
 
-/* A pseudo ostream that takes yydebug_ into account.  */
-# define YYCDEBUG if (yydebug_) (*yycdebug_)
-
 /* Enable debugging if requested.  */
 #if YYDEBUG
 
+/* A pseudo ostream that takes yydebug_ into account.  */
+# define YYCDEBUG if (yydebug_) (*yycdebug_)
+
 # define YY_SYMBOL_PRINT(Title, Type, Value, Location) \
 do {                                                   \
   if (yydebug_)                                                \
@@ -370,6 +369,7 @@ do {                                        \
 
 #endif /* !YYDEBUG */
 
+#define YYCDEBUG       if (false) std::cerr
 #define YYACCEPT       goto yyacceptlab
 #define YYABORT                goto yyabortlab
 #define YYERROR                goto yyerrorlab
@@ -418,9 +418,12 @@ namespace ]b4_namespace[
 #endif
 
   /// Build a parser object.
-  ]b4_parser_class_name::b4_parser_class_name[ (]b4_parse_param_decl[)
-    : yydebug_ (false),
-      yycdebug_ (&std::cerr)]b4_parse_param_cons[
+  ]b4_parser_class_name::b4_parser_class_name[ 
(]b4_parse_param_decl[)]m4_ifset([b4_parse_param], [
+    :])[
+#if YYDEBUG
+    ]m4_ifset([b4_parse_param], [  ], [ :])[yydebug_ (false),
+      yycdebug_ (&std::cerr)]m4_ifset([b4_parse_param], [,])[
+#endif]b4_parse_param_cons[
   {
   }
 
@@ -458,7 +461,7 @@ namespace ]b4_namespace[
     yy_symbol_value_print_ (yytype, yyvaluep, yylocationp);
     *yycdebug_ << ')';
   }
-#endif /* ! YYDEBUG */
+#endif
 
   void
   ]b4_parser_class_name[::yydestruct_ (const char* yymsg,
@@ -486,6 +489,7 @@ namespace ]b4_namespace[
     yylocation_stack_.pop (n);
   }
 
+#if YYDEBUG
   std::ostream&
   ]b4_parser_class_name[::debug_stream () const
   {
@@ -510,7 +514,7 @@ namespace ]b4_namespace[
   {
     yydebug_ = l;
   }
-
+#endif
 
   int
   ]b4_parser_class_name[::parse ()
--- tests/calc.at       8 Jul 2006 20:38:14 -0000       1.90
+++ tests/calc.at       13 Sep 2006 17:39:54 -0000
@@ -162,7 +162,9 @@ int
 yyparse (AT_PARAM_IF([semantic_value *result, int *count]))
 {
   AT_NAME_PREFIX::parser parser[]AT_PARAM_IF([ (result, count)]);
-  parser.set_debug_level (!!YYDEBUG);
+#if YYDEBUG
+  parser.set_debug_level (1);
+#endif
   return parser.parse ();
 }
 ],
@@ -642,11 +644,11 @@ AT_BANNER([[Simple GLR C++ Calculator.]]
 m4_define([AT_CHECK_CALC_GLR_CC],
 [AT_CHECK_CALC([%skeleton "glr.cc" %defines %locations] $@)])
 
-#AT_CHECK_CALC_GLR_CC([])
-#AT_CHECK_CALC_GLR_CC([%error-verbose %name-prefix="calc" %verbose %yacc])
+AT_CHECK_CALC_GLR_CC([])
+AT_CHECK_CALC_GLR_CC([%error-verbose %name-prefix="calc" %verbose %yacc])
 
-# AT_CHECK_CALC_GLR_CC([%debug])
-#AT_CHECK_CALC_GLR_CC([%error-verbose %debug %name-prefix="calc" %verbose 
%yacc])
+AT_CHECK_CALC_GLR_CC([%debug])
+AT_CHECK_CALC_GLR_CC([%error-verbose %debug %name-prefix="calc" %verbose 
%yacc])
 
 AT_CHECK_CALC_GLR_CC([%pure-parser %error-verbose %debug %name-prefix="calc" 
%verbose %yacc])
 
--- tests/regression.at 18 Aug 2006 21:43:18 -0000      1.107
+++ tests/regression.at 13 Sep 2006 17:39:54 -0000
@@ -881,7 +881,9 @@ int
 yyparse ()
 {
   yy::parser parser;
-  parser.set_debug_level (!!YYDEBUG);
+#if YYDEBUG
+  parser.set_debug_level (YYDEBUG);
+#endif
   return parser.parse ();
 }
 ],