The forthcoming automove feature, to be properly checked, will require
that we can rely on the value of a moved-from string, which is not
something the C++ standard guarantees. So introduce our own wrapper.
Suggested by Frank Heckenbach.
https://lists.gnu.org/archive/html/bison-patches/2018-09/msg00111.html
* tests/c++.at (Variants): Introduce and use a new 'string' class.
The 'Variants' tests are well suited to check support for move, and in
particular for the forthcoming automove feature. But the tests were
written to show the best practice in C++98, using swap:
list "," item { std::swap ($$, $1); $$.push_back ($3); }
This cannot work with std::move. So, make this example simpler, based
on regular assignment instead of swap, which is a regression for
C++98 (as the new traces show), but will be an improvement for modern
C++ with automove.
* tests/c++.at (Variants): Stop using swap.
We don't generate a header file, so remove the 'require' code section.
Adjust expectations.
The code was already using midrule only, never mid_rule. This is
simpler to remember, and matches a similar change we made from
look-ahead to lookahead.
* NEWS, doc/bison.texi, src/reader.c, src/scan-code.h, src/scan-code.l
* tests/actions.at, tests/c++.at, tests/existing.at: here.
This is much of course more efficient than in the matrix of the CI (or
on our own machines), but a bit more tedious.
* configure.ac (CXX03_CXXFLAGS, CXX11_CXXFLAGS, CXX14_CXXFLAGS)
(CXX17_CXXFLAGS, CXX2A_CXXFLAGS, STDCXX_FLAGS): New.
* tests/atlocal.in: Receive them.
* tests/local.at (AT_FOR_EACH_CXX): New.
* tests/c++.at: Use AT_FOR_EACH_CXX.
Reported by Frank Heckenbach.
http://lists.gnu.org/archive/html/bug-bison/2018-03/msg00002.html
Actually the assignment operator should never be needed: the C++98
requirements for vector::push_back is CopyInsertable, which does not require
an assignment operator. However, libstdc++ shipped with GCC up to (and
including) 6 uses the assignment operator (which affects Clang on top of
libstdc++, but also ICC). So let's keep it for legacy C++.
See https://gcc.godbolt.org/z/q0XXmC.
* data/lalr1.cc (stack_symbol_type::operator=): Remove.
* data/c++.m4 (basic_symbol::operator=): Ditto.
* tests/c++.at (C++ Variant-based Symbols Unit Tests): Adjust.
Modern C++ (i.e., C++11 and later) introduced "move only" types: types such
as std::unique_ptr<T> that can never be duplicated. They must never be
copied (by assignments and constructors), they must be "moved". The
implementation of lalr1.cc used to copy symbols (including their semantic
values). This commit ensures that values are only moved in modern C++, yet
remain compatible with C++98/C++03.
Suggested by Frank Heckenbach, who provided a full implementation on
top of C++17's std::variant.
See http://lists.gnu.org/archive/html/bug-bison/2018-03/msg00002.html,
and https://lists.gnu.org/archive/html/bison-patches/2018-04/msg00002.html.
Symbols (terminal/non terminal) are handled by several functions that used
to take const-refs, which resulted eventually in a copy pushed on the stack.
With modern C++ (C++11 and later) the callers must use std::move, and the
callees must take their arguments as rvalue refs (foo&&). In order to avoid
duplicating these functions to support both legacy C++ and modern C++, let's
introduce macros (YY_MOVE, YY_RVREF, etc.) that rely on copy-semantics for
C++98/03, and move-semantics for modern C++.
That's easy for inner types, when the parser's functions pass arguments to
each other. Functions facing the user (make_NUMBER, make_STRING, etc.)
should support both rvalue-refs (for instance to support move-only types:
make_INT (std::make_unique<int> (1))), and lvalue-refs (so that we can pass
a variable: make_INT (my_int)). To avoid the multiplication of the
signatures (there is also the location), let's take the argument by value.
See:
https://lists.gnu.org/archive/html/bison-patches/2018-09/msg00024.html.
* data/c++.m4 (b4_cxx_portability): New.
(basic_symbol): In C++11, replace copy-ctors with move-ctors.
In C++11, replace copies with moves.
* data/lalr1.cc (stack_symbol_type, yypush_): Likewise.
Use YY_MOVE to avoid useless copies.
* data/variant.hh (variant): Support move-semantics.
(make_SYMBOL): In C++11, in order to support both read-only lvalues,
and rvalues, take the argument as a copy.
* data/stack.hh (yypush_): Use rvalue-refs in C++11.
* tests/c++.at: Use move semantics.
* tests/headers.at: Adjust to the new macros (YY_MOVE, etc.).
* configure.ac (CXX98_CXXFLAGS, CXX11_CXXFLAGS, CXX14_CXXFLAGS)
(CXX17_CXXFLAGS, ENABLE_CXX11): New.
* tests/atlocal.in: Receive them.
* examples/variant.yy: Don't define things in std.
* examples/variant-11.test, examples/variant-11.yy: New.
Check the support of move-only types.
* examples/README, examples/local.mk: Adjust.
With recent compilers:
input.yy:49:5: error: definition of implicit copy assignment
operator for 'Object' is deprecated because
it has a user-declared destructor
[-Werror,-Wdeprecated]
~Object ()
^
input.yy:130:35: note: in implicit copy assignment operator for
'Object' first required here
{ yylhs.value.as< Object > () = yystack_[0].value.as< Object > (); }
* tests/c++.at (Object): Add missing assignment operator.
Sometimes `inline` would be used in *.cc files on symbols that are not
exported (useless but harmless), and sometimes on exported symbols
such as the constructor of syntax_error (harmful: linking fails).
Reported several times, including:
- by Dennis T
http://lists.gnu.org/archive/html/bug-bison/2016-03/msg00002.html
- by Frank Heckenbach
https://savannah.gnu.org/patch/?9616
* data/c++.m4 (b4_inline): New: expands to `inline` or nothing.
Use it where appropriate.
* data/lalr1.cc: Use it where appropriate.
* tests/c++.at (Syntax error as exception): Put the scanner in another
compilation unit to exercise the constructor of syntax_error.
* data/lalr1.cc, doc/bison.texi, etc/bench.pl.in, examples/variant.yy,
* tests/actions.at, tests/atlocal.in, tests/c++.at, tests/headers.at,
* tests/local.at, tests/types.at:
Don't use std::endl, it flushes uselessly, and is considered bad
style.
Sun C 5.13 SunOS_sparc 2014/10/20 reports errors on tests 430-432.
Reported by Dennis Clarke.
<http://lists.gnu.org/archive/html/bug-bison/2015-01/msg00087.html>
* tests/c++.at (Variants): Be sure to emit operator<< before using it:
use "%code top" rather than "%code".
Prefer std::vector to std::list.
Do not define anything in std::, to avoid undefined behavior.
This is consistent with what is done with yacc.c and glr.c. Because
it also avoids that the stack needs to be resized very soon, it should
help keeping tests about destructors more reliable.
Indeed, if the stack is created too small, very soon the C++ library
needs to enlarge it, which means creating a new one, copying the
elements from the initial one onto it, and then destroy the elements
of the initial stack: that would be a spurious call to a destructor.
Reported by Thomas Jahns.
http://lists.gnu.org/archive/html/bug-bison/2015-01/msg00059.html
* data/stack.hh (stack::stack): Reserve 200 slots.
* tests/c++.at: Remove traces of stack expansions.
When using variants, destructors generate invalid code.
<http://lists.gnu.org/archive/html/bug-bison/2014-09/msg00005.html>
Reported by Michael Catanzaro.
* data/c++.m4 (~basic_symbol): b4_symbol_foreach works on yysym:
define it.
* tests/c++.at (Variants): Check it.
During error recovery, when discarding the lookeahead, we don't
destroy it, which is caught by parse.assert assertions.
Reported by Antonio Silva Correia.
With an analysis and suggested patch from Michel d'Hooge.
<http://savannah.gnu.org/support/?108481>
* tests/c++.at (Variants): Strengthen the test to try syntax errors
with discarded lookahead.
There are no support for += between locations, and some comments are wrong.
Reported by Alexandre Duret-Lutz.
* data/location.cc: Fix.
* doc/bison.texi: Document.
* tests/c++.at: Check.
When variant are enabled, the yylhs variable (the left-hand side of
the rule being reduced, i.e. $$ and @$) is explicitly destroyed when
YYERROR is called. This is because before running the user code, $$
is initialized, so that the user can properly use it.
However, when quitting yyparse, yylhs is also reclaimed by the C++
compiler: the variable goes out of scope.
Instead of trying to be too smart, let the compiler do its job: reduce
the scope of yylhs to exactly the reduction. This way, whatever the
type of scope exit (regular, exception, return, goto...) this variable
will be properly reclaimed.
Reported by Paolo Simone Gasparello.
<http://lists.gnu.org/archive/html/bug-bison/2013-10/msg00003.html>
* data/lalr1.cc (yyparse): Reduce the scope of yylhs.
* tests/c++.at: We now pass this test.
When variant are enabled, the yylhs variable (the left-hand side of
the rule being reduced, i.e. $$ and @$) is explicitly destroyed when
YYERROR is called. This is because before running the user code, $$
is initialized, so that the user can properly use it.
However, when quitting yyparse, yylhs is also reclaimed by the C++
compiler: the variable goes out of scope.
This was not detected by the test suite because (i) the Object tracker
was too weak, and (ii) the problem does not show when there is error
recovery.
Reported by Paolo Simone Gasparello.
<http://lists.gnu.org/archive/html/bug-bison/2013-10/msg00003.html>
* tests/c++.at (Exception safety): Improve the objects logger to make
sure that we never destroy twice an object.
Also track copy-constructors.
Use a set instead of a list.
Display the logs before running the function body, this is more
useful in case of failure.
Generalize to track with and without error recovery.
Some tests now fail when compiled with G++ 4.3 or 4.4 on MacPorts.
* tests/local.at (AT_SKIP_IF_EXCEPTION_SUPPORT_IS_POOR): New.
* tests/c++.at (Exception safety): Use it.
* tests/c++.at (Exception safety): In variant mode $$ is an instance
of Object. Assigning YY_NULL in C++98 is incorrect, but behaves ok,
as it assigns YY_NULL=0 using Object::operator= (char v). It is wrong
in C++11 as there is operator for "$$ = nullptr".
Reported by Daniel Frużyński.
http://lists.gnu.org/archive/html/bug-bison/2013-02/msg00000.html
* data/location.cc (position::columns, position::lines): Check for
underflow.
Fix some weird function signatures.
(location): Accept signed integers as arguments where appropriate.
Add operator- and operator+=.
* doc/bison.texi (C++ position, C++ location): Various fixes
and completion.
* tests/c++.at (C++ Locations): New tests.
* tests/c++.at, tests/input.at: Use "%define api.namespace {foo}" instead
of using quotes.
* tests/local.at (AT_SETUP_STRIP, AT_NAME_PREFIX): Recognize uses of
braces instead of quotes.
* doc/bison.texi: Use braces for api.namespace's values.
* tests/c++.at (Object): Somehow instances of Object were assigned
YY_NULL, which is 0 most of the time (that case passes), but is
nullptr in C++11, and there is nothing in Object to support such an
assignment (failure). Use 0 as value, and provide the needed
assignment operator.
Also, use a more natural order within the class definition.
* tests/c++.at (C++ Variant-based Symbol, Variants): Here. Rename the
generated input files to use .y instead of .yy, as a requirement for using
AT_FULL_COMPILE instead of a combination of AT_BISON_CHECK and
AT_BISON_COMPILE_CXX.
This is based on what is recommended by both Scott Meyers, in 'Effective
C++', and Andrei Alexandrescu and Herb Sutter in 'C++ Coding Standards'.
Use a static_cast on void* rather than directly use a reinterpret_cast,
which can have nefarious effects on objects. However, even though following
this guideline is good practice in general, I am not quite sure how relevant
it is when applied to conversions from POD to objects. Actually, it might
very well be the opposite: isn't this exactly what reinterpret_cast is for?
What we really want *is* to transmit the memory map as a series of bytes,
which, if I am correct, falls into the kind of "low level" hack for which
this cast is meant.
In any case, this silences the warning, which will be greatly appreciated by
anyone using variants with a compiler supporting -fstrict-aliasing.
* data/variant.hh (as): Here.
* tests/c++.at (Exception safety, C++ Variant-based Symbols, Variants):
Don't use NO_STRICT_ALIAS_CXXFLAGS (revert commit ddb9db15), as type punning
is no longer an issue.
* tests/atlocal.in, configure.ac (NO_STRICT_ALIAS_CXXFLAGS): Remove
definition.
* examples/local.mk (NO_STRICT_ALIAS_CXXFLAGS): Remove from AM_CXXFLAGS.
* doc/bison.texi: Don't mention type punning issues.
* cfg.mk: Ignore strcmp in local.at.
* tests/conflicts.at: Use AT_PARSER_CHECK.
* tests/regression.at: Preserve the exit status of the generated parsers.
* tests/local.mk ($(TESTSUITE)): Map @tb@ to a tabulation.
* tests/c++.at, tests/input.at, tests/regression.at: Use @tb@.
* cfg.mk: (space-tab): There are no longer exceptions.
* tests/local.at: (Slightly) improve the regexp by escaping '.'
when it denotes a point.
(AT_VARIANT_IF): New.
* tests/c++.at (Exception Safety): Run it for variants too.
Many tests were using %defines because C++ skeletons used to require
it.
* tests/actions.at, tests/c++.at, tests/input.at, tests/regression.at:
Remove useless %defines.
The current approach was too adhoc: the symbols were not sufficiently
self-contained, in particular wrt memory management. The "new"
guideline is the one that should have been followed from the start:
let the symbols handle themslves, instead of leaving their users to
it. It was justified by the will to avoid gratuitious moves and
copies, but the current approach does not seem to be slower, yet it
will probably be simpler to adjust to support move semantics from
C++11.
The documentation says that the %parse-param are available from the
%destructor. In retrospect, that was a silly design decision, which
we can break for variants, as its a new feature. It should be phased
out for non-variants too.
* data/variant.hh: A variant never knows if it stores something or
not, it is up to its users to store this information.
Yet, in parse.assert mode, make sure the empty/filled variants
are properly used.
(b4_symbol_constructor_define_): Don't call directly the symbol
constructor, to save a useless temporary.
* data/stack.hh (push): Steal the pushed value instead of duplicating
it.
This will simplify the callers of push, who handled this "move"
approach themselves.
* data/c++.m4 (basic_symbol): Let -1, as kind, denote the fact that
a symbol is empty.
This is needed for instance when shifting the lookahead: yyla
is given as argument to "push", and its value is then moved on
the stack. But then yyla must be declared "empty" so that its
destructor won't be called.
(basic_symbol::move): New.
Move the responsibility of calling the destructor from yy_destroy
to ~basic_symbol in the case of variants.
* data/lalr1.cc (stack_symbol_type): Now a derived class from its
previous value, so that we can add a constructor from a symbol_type.
(by_state): State -1 means empty.
(yypush_): Factor, by calling one overload from the other one, and
using the new semantics of stack::push.
No longer reclaim by hand the memory from rhs symbols, since now
that we store objects with proper destructors, they will be reclaimed
automatically.
Conversely, be sure to delete yylhs.
* tests/c++.at (C++ Variant-based Symbols): New "unit" test for
symbols.
* tests/local.at (AT_NAME_PREFIX): Also match api.namespace.
(AT_MAIN_DEFINE): Take it into account.
* tests/c++.at, tests/headers.at: Use AT_NAME_PREFIX.
(AT_CHECK_NAMESPACE): Rename as...
(AT_TEST): this.
