Re: RFC: c++: provide control over the stack.hh file name

[Akim Demaille]

Hi

> Le 24 sept. 2018 à 22:36, Akim Demaille <address@hidden> a écrit :
> 
> commit d3c59fc9587f17e85d888884d57ff8d18fd9f9ef
> Author: Akim Demaille <address@hidden>
> Date:   Sun Sep 23 17:59:46 2018 +0200
> 
>    WIP: c++: provide control over the stack.hh file name
> 
>    It was not a good idea to generate the file stack.hh.  It never was.
>    But now we have to deal with backward compatibility: if we stop
>    generating it, the build system of some build system will probably
>    break.
> 
>    So offer the user a means to (i) decide what the name of the output
>    file should be, and (ii) not generate this file at all (its content
>    will be inline where the parser is defined).
> 
>    * data/c++.m4 (api.stack.file): Defaults to stack.hh.
>    * data/stack.hh: Generate the file only if api.stack.file is not
>    empty.
>    In that case, use it as file name.
>    * data/lalr1.cc: Adjust to include the right file, or to include
>    the definition of stack.
>    * tests/calc.at, tests/output.at: Exercise api.stack.file.

Here is an updated version of this patch.  It’s only an improvement
of the previous version, it did not change things such as the name
of the variable.  I am still undecided on this regard, as I find it
nice that related variables are grouped together, and for instance
with api.location, it feels nice to have api.location.file.  Yet I’m
not entire sure a file name fits into what people call APIs.

My idea is: let’s finish the series of patches (location and position
follow, and there’s probably some cleaning that will follow) and
document, and _then_, take a step back to see how good/bad it looks.


commit 156c61dcfd0daf4872fde496f977322258c2bf23
Author: Akim Demaille <address@hidden>
Date:   Sun Sep 23 17:59:46 2018 +0200

    c++: provide control over the stack.hh file name
    
    It was not a good idea to generate the file stack.hh.  It never was.
    But now we have to deal with backward compatibility: if we stop
    generating it, the build system of some build system will probably
    break.
    
    So offer the user a means to (i) decide what the name of the output
    file should be, and (ii) not generate this file at all (its content
    will be inline where the parser is defined).
    
    * data/lalr1.cc (_b4_percent_define_check_file_warn)
    (_b4_percent_define_check_file): New.
    * data/stack.hh: Generate the file only if api.stack.file is not
    empty.
    In that case, use it as file name.
    * data/lalr1.cc: Adjust to include the right file, or to include
    the definition of stack.
    * tests/calc.at, tests/output.at: Exercise api.stack.file.

diff --git a/data/lalr1.cc b/data/lalr1.cc
index af9c9e2a..413a4cab 100644
--- a/data/lalr1.cc
+++ b/data/lalr1.cc
@@ -20,6 +20,39 @@ m4_include(b4_pkgdatadir/[c++.m4])
 # api.value.type=variant is valid.
 m4_define([b4_value_type_setup_variant])
 
+
+# _b4_percent_define_check_file_warn(VARIABLE)
+# --------------------------------------------
+# Warn about %define variable VARIABLE having an incorrect
+# value.
+m4_define([_b4_percent_define_check_file_warn],
+[b4_warn_at(b4_percent_define_get_loc([$1]),
+            [[%%define variable '%s' requires 'none' or '"..."' values]],
+            [$1])])
+
+
+# _b4_percent_define_check_file(MACRO, VARIABLE, DEFAULT)
+# -------------------------------------------------------
+# If the %define variable VARIABLE:
+# - is undefined, then if DEFAULT is non-empty, define MACRO to DEFAULT
+# - is a string, define MACRO to its value
+# - is the keyword 'none', do nothing
+# - otherwise, warn about the incorrect value.
+m4_define([_b4_percent_define_check_file],
+[b4_percent_define_ifdef([$2],
+  [m4_case(b4_percent_define_get_kind([$2]),
+    [string],
+         [m4_define([$1], b4_percent_define_get([$2]))],
+    [keyword],
+         [m4_if(b4_percent_define_get([$2]), [none], [],
+                [_b4_percent_define_check_file_warn([$2])])],
+    [_b4_percent_define_check_file_warn([$2])])
+   ],
+   [m4_ifval([$3],
+             [errprint(DEFINE)m4_define([$1], [$3])])])
+])
+
+
 # b4_integral_parser_table_declare(TABLE-NAME, CONTENT, COMMENT)
 # --------------------------------------------------------------
 # Declare "parser::yy<TABLE-NAME>_" whose contents is CONTENT.
@@ -161,8 +194,9 @@ m4_define([b4_shared_declarations],
 # include <vector>
 
 ]b4_cxx_portability[
+]m4_ifdef([b4_stack_file],
+          [[# include "]b4_stack_file["]])[
 ]b4_defines_if([[
-# include "stack.hh"
 ]b4_bison_locations_if([[# include "location.hh"]])])[
 ]b4_variant_if([b4_variant_includes])[
 
@@ -173,9 +207,9 @@ m4_define([b4_shared_declarations],
 
 ]b4_namespace_open[
 
+]m4_ifdef([b4_stack_file], [], [b4_stack_define])[
 ]b4_defines_if([],
-[b4_stack_define
-b4_bison_locations_if([b4_position_define
+[b4_bison_locations_if([b4_position_define
 b4_location_define])])[
 
 ]b4_variant_if([b4_variant_define])[
diff --git a/data/stack.hh b/data/stack.hh
index 0ee2208b..0e6fca31 100644
--- a/data/stack.hh
+++ b/data/stack.hh
@@ -18,6 +18,14 @@
 m4_pushdef([b4_copyright_years],
            [2002-2015, 2018])
 
+# b4_stack_file
+# -------------
+# Name of the file containing the stack class, if we want this file.
+_b4_percent_define_check_file([b4_stack_file],
+                              [[api.stack.file]],
+                              b4_defines_if([[stack.hh]]))
+
+
 # b4_stack_define
 # ---------------
 m4_define([b4_stack_define],
@@ -129,16 +137,17 @@ m4_define([b4_stack_define],
   };
 ]])
 
-b4_defines_if(
-[b4_output_begin([b4_dir_prefix[]stack.hh])
-b4_copyright([Stack handling for Bison parsers in C++])[
+
+m4_ifdef([b4_stack_file],
+[b4_output_begin([b4_dir_prefix[]b4_stack_file])[
+]b4_copyright([Stack handling for Bison parsers in C++])[
 
 /**
- ** \file ]b4_dir_prefix[stack.hh
+ ** \file ]b4_dir_prefix[]b4_stack_file[
  ** Define the ]b4_namespace_ref[::stack class.
  */
 
-]b4_cpp_guard_open([b4_dir_prefix[]stack.hh])[
+]b4_cpp_guard_open([b4_dir_prefix[]b4_stack_file])[
 
 # include <vector>
 
@@ -148,7 +157,8 @@ b4_copyright([Stack handling for Bison parsers in C++])[
 ]b4_stack_define[
 ]b4_namespace_close[
 
-]b4_cpp_guard_close([b4_dir_prefix[]stack.hh])[
-]b4_output_end])
+]b4_cpp_guard_close([b4_dir_prefix[]b4_stack_file])[
+]b4_output_end[
+]])
 
 m4_popdef([b4_copyright_years])
diff --git a/tests/calc.at b/tests/calc.at
index a75a1bc6..4f20dbd7 100644
--- a/tests/calc.at
+++ b/tests/calc.at
@@ -696,6 +696,9 @@ AT_CHECK_CALC_LALR1_CC([%defines %locations %define 
parse.error verbose %debug %
 AT_CHECK_CALC_LALR1_CC([%define parse.error verbose %debug %define api.prefix 
{calc} %verbose %yacc %parse-param {semantic_value *result} %parse-param {int 
*count}])
 AT_CHECK_CALC_LALR1_CC([%defines %locations %define parse.error verbose %debug 
%define api.prefix {calc} %verbose %yacc %parse-param {semantic_value *result} 
%parse-param {int *count}])
 
+AT_CHECK_CALC_LALR1_CC([%locations %define api.stack.file "my-stack.hh"])
+AT_CHECK_CALC_LALR1_CC([%locations %defines %define api.stack.file none])
+AT_CHECK_CALC_LALR1_CC([%defines %locations %define api.stack.file 
"my-stack.hh"])
 
 
 # --------------------------- #
diff --git a/tests/input.at b/tests/input.at
index 527c0398..1643de98 100644
--- a/tests/input.at
+++ b/tests/input.at
@@ -1624,6 +1624,28 @@ input.y:1.9-21:     accepted value: 'both'
 
 AT_CLEANUP
 
+
+
+## ------------------------ ##
+## %define file variables.  ##
+## ------------------------ ##
+
+AT_SETUP([["%define" file variables]])
+
+AT_DATA([[input.y]],
+[[%skeleton "lalr1.cc"
+%define api.stack.file bogus
+%%
+start: %empty;
+]])
+AT_BISON_CHECK([[-fcaret input.y]], [[0]], [[]],
+[[input.y:2.9-22: warning: %define variable 'api.stack.file' requires 'none' 
or '"..."' values [-Wother]
+ %define api.stack.file bogus
+         ^^^^^^^^^^^^^^
+]])
+
+AT_CLEANUP
+
 ## -------------------------------- ##
 ## %define backward compatibility.  ##
 ## -------------------------------- ##
diff --git a/tests/output.at b/tests/output.at
index 6bee3147..c161d378 100644
--- a/tests/output.at
+++ b/tests/output.at
@@ -145,6 +145,9 @@ AT_CHECK([grep 'include .subdir/' $1.cc], 1, [])
 AT_CHECK([grep 'include .subdir/' $1.hh], 1, [])
 ])
 
+AT_CHECK_OUTPUT([foo.yy], [%skeleton "lalr1.cc"], [],
+                [foo.tab.cc])
+
 AT_CHECK_OUTPUT([foo.yy], [%skeleton "lalr1.cc" %verbose], [],
                 [foo.output foo.tab.cc])
 
@@ -176,6 +179,22 @@ AT_CHECK_OUTPUT([gram_dir/foo.yy],
                 [],
                 [output_dir/foo.output output_dir/foo.tab.cc 
output_dir/foo.tab.hh output_dir/location.hh output_dir/position.hh 
output_dir/stack.hh])
 
+# api.stack.file.
+AT_CHECK_OUTPUT([foo.yy],
+                [%skeleton "lalr1.cc" %define api.stack.file "foo.sta.hh"],
+                [],
+                [foo.sta.hh foo.tab.cc])
+
+AT_CHECK_OUTPUT([foo.yy],
+                [%skeleton "lalr1.cc" %defines %define api.stack.file none],
+                [],
+                [foo.tab.cc foo.tab.hh])
+
+AT_CHECK_OUTPUT([gram_dir/foo.yy],
+                [%skeleton "lalr1.cc" %verbose %defines %define api.stack.file 
"foo.sta.hh" %file-prefix "output_dir/foo"],
+                [],
+                [output_dir/foo.output output_dir/foo.sta.hh 
output_dir/foo.tab.cc output_dir/foo.tab.hh])
+
 
 # AT_CHECK_CONFLICTING_OUTPUT(INPUT-FILE, DIRECTIVES, FLAGS, STDERR,
 #                             [EXIT-STATUS])