Re: Bison 3.2.90 released [beta]

[Derek Clegg]

Thanks for this fix. I had some problems applying the patch, so I’ll wait until 
the next release candidate to test it. It looks okay, however, based on a brief 
inspection. One thing:

> if test "$enable_gcc_warnings" = yes; then
>   warn_common='-Wall -Wextra -Wno-sign-compare -Wcast-align
>     -fparse-all-comments -Wdocumentation
> -    -Wformat -Wnull-dereference -Wpointer-arith -Wshadow -Wwrite-strings'
> +    -Wformat -Wnull-dereference -Wpointer-arith -Wshadow
> +    -Wundefined-func-template -Wwrite-strings'
>   warn_c='-Wbad-function-cast -Wstrict-prototypes'
>   warn_cxx='-Wextra-semi -Wnoexcept'
>   # Warnings for the test suite only.

I would expect “-Wundefined-func-template” to appear in “warn_cxx”, not 
“warn_common”. But I may not fully understand what’s going on here.

Thanks!
Derek

> On Jan 14, 2019, at 10:30 PM, Akim Demaille <address@hidden> wrote:
> 
> Hi Derek,
> 
>> Le 13 janv. 2019 à 23:01, Derek Clegg <address@hidden> a écrit :
>> 
>> This is a more serious problem for me:
>> 
>> aux/parser-internal.h:767:7: error: instantiation of function
>>     'yy::parser::basic_symbol<yy::parser::by_type>::basic_symbol' required
>>     here, but no definition is available [-Werror,-Wundefined-func-template]
>>     symbol_type () {}
>>     ^
>> aux/parser-internal.h:530:7: note: forward declaration of template entity is
>>     here
>>     basic_symbol ();
>>     ^
>> aux/parser-internal.h:767:7: note: add an explicit instantiation declaration 
>> to
>>     suppress this warning if
>>     'yy::parser::basic_symbol<yy::parser::by_type>::basic_symbol' is
>>     explicitly instantiated in another translation unit
>>     symbol_type () {}
>>     ^
>> 1 error generated.
>> 
>> I don’t know think it’s possible to declare a forward declaration of a 
>> template entity within a class, which makes the fix nontrivial. The only fix 
>> I can see is to move the definitions of the symbol_type methods outside of 
>> the header file and into the .cc file (which I think would be clearer, in 
>> any case).
> 
> That it would be clearer, I agree, it was like this initially.  But it's too 
> much effort to maintain it this way, and no-one answered my question about 
> that:
> 
> https://lists.gnu.org/archive/html/bison-patches/2018-12/msg00058.html
> 
> So I installed this one:
> 
> https://lists.gnu.org/archive/html/bison-patches/2018-12/msg00075.html
> 
> and several others afterwards.
> 
> So I'd rather not go "backwards" (in the chronological sense, not judgmental 
> on the style, I tend to agree with you).
> 
> I'll install this, once validated by the CI.
> 
> Thanks a lot for this report.
> 
> 
> 
> commit a049509d0437046e57a0e96f71452ddb33f8eecc
> Author: Akim Demaille <address@hidden>
> Date:   Mon Jan 14 19:57:02 2019 +0100
> 
>    c++: avoid -Wundefined-func-template warnings from clang
> 
>    Reported by Derek Clegg.
>    http://lists.gnu.org/archive/html/bug-bison/2019-01/msg00006.html
> 
>    Clang does not like this:
> 
>        template <typename D>
>        struct basic_symbol : D
>        {
>          basic_symbol();
>        };
> 
>        struct by_type {};
> 
>        struct symbol_type : basic_symbol<by_type>
>        {
>          symbol_type(){}
>        };
> 
>    It gives:
> 
>        $ clang++-mp-7.0 -Wundefined-func-template foo.cc -c
>        foo.cc:11:3: warning: instantiation of function 
> 'basic_symbol<by_type>::basic_symbol'
>                     required here, but no definition is available 
> [-Wundefined-func-template]
>          symbol_type(){}
>          ^
>        foo.cc:4:3: note: forward declaration of template entity is here
>          basic_symbol();
>          ^
>        foo.cc:11:3: note: add an explicit instantiation declaration to 
> suppress this warning
>                     if 'basic_symbol<by_type>::basic_symbol' is explicitly 
> instantiated in
>                     another translation unit
>          symbol_type(){}
>          ^
>        1 warning generated.
> 
>    The same applies for the basic_symbol's destructor and `clear()`.
> 
>    * configure.ac (warn_cxx): Add -Wundefined-func-template.
>    This triggered one failure in the test suite:
>    * tests/headers.at (Sane headers): here, where we check that we can
>    compile the generated headers in other compilation units than the
>    parser's.
>    Add a variant type to make sure that basic_symbol and symbol_type are
>    properly generated in this case.
>    * data/skeletons/c++.m4 (basic_symbol): Inline the definitions of the
>    destructor and of `clear` in the class definition.
> 
> diff --git a/configure.ac b/configure.ac
> index e60af602..b7ba45dc 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -96,7 +96,8 @@ AM_CONDITIONAL([ENABLE_GCC_WARNINGS], [test 
> "$enable_gcc_warnings" = yes])
> if test "$enable_gcc_warnings" = yes; then
>   warn_common='-Wall -Wextra -Wno-sign-compare -Wcast-align
>     -fparse-all-comments -Wdocumentation
> -    -Wformat -Wnull-dereference -Wpointer-arith -Wshadow -Wwrite-strings'
> +    -Wformat -Wnull-dereference -Wpointer-arith -Wshadow
> +    -Wundefined-func-template -Wwrite-strings'
>   warn_c='-Wbad-function-cast -Wstrict-prototypes'
>   warn_cxx='-Wextra-semi -Wnoexcept'
>   # Warnings for the test suite only.
> diff --git a/data/skeletons/c++.m4 b/data/skeletons/c++.m4
> index c985cb98..6f2d75d2 100644
> --- a/data/skeletons/c++.m4
> +++ b/data/skeletons/c++.m4
> @@ -260,7 +260,10 @@ m4_define([b4_symbol_type_define],
>       typedef Base super_type;
> 
>       /// Default constructor.
> -      basic_symbol ();
> +      basic_symbol ()
> +        : value ()]b4_locations_if([
> +        , location ()])[
> +      {}
> 
> #if 201103L <= YY_CPLUSPLUS
>       /// Move constructor.
> @@ -282,10 +285,29 @@ m4_define([b4_symbol_type_define],
>                     YY_RVREF (location_type) l])[);
> ]])[
>       /// Destroy the symbol.
> -      ~basic_symbol ();
> +      ~basic_symbol ()
> +      {
> +        clear ();
> +      }
> 
>       /// Destroy contents, and record that is empty.
> -      void clear ();
> +      void clear ()
> +      {]b4_variant_if([[
> +        // User destructor.
> +        symbol_number_type yytype = this->type_get ();
> +        basic_symbol<Base>& yysym = *this;
> +        (void) yysym;
> +        switch (yytype)
> +        {
> +]b4_symbol_foreach([b4_symbol_destructor])dnl
> +[       default:
> +          break;
> +        }
> +
> +        // Type destructor.
> +]b4_symbol_variant([[yytype]], [[value]], [[template destroy]])])[
> +        Base::clear ();
> +      }
> 
>       /// Whether empty.
>       bool empty () const YY_NOEXCEPT;
> @@ -365,12 +387,6 @@ m4_define([b4_symbol_type_define],
> # Provide the implementation needed by the public types.
> m4_define([b4_public_types_define],
> [[  // basic_symbol.
> -  template <typename Base>
> -  ]b4_parser_class[::basic_symbol<Base>::basic_symbol ()
> -    : value ()]b4_locations_if([
> -    , location ()])[
> -  {}
> -
> #if 201103L <= YY_CPLUSPLUS
>   template <typename Base>
>   ]b4_parser_class[::basic_symbol<Base>::basic_symbol (basic_symbol&& that)
> @@ -416,32 +432,6 @@ m4_define([b4_public_types_define],
>     (void) v;
>     ]b4_symbol_variant([this->type_get ()], [value], [YY_MOVE_OR_COPY], 
> [YY_MOVE (v)])])[}]])[
> 
> -  template <typename Base>
> -  ]b4_parser_class[::basic_symbol<Base>::~basic_symbol ()
> -  {
> -    clear ();
> -  }
> -
> -  template <typename Base>
> -  void
> -  ]b4_parser_class[::basic_symbol<Base>::clear ()
> -  {]b4_variant_if([[
> -    // User destructor.
> -    symbol_number_type yytype = this->type_get ();
> -    basic_symbol<Base>& yysym = *this;
> -    (void) yysym;
> -    switch (yytype)
> -    {
> -]b4_symbol_foreach([b4_symbol_destructor])dnl
> -[   default:
> -      break;
> -    }
> -
> -    // Type destructor.
> -  ]b4_symbol_variant([[yytype]], [[value]], [[template destroy]])])[
> -    Base::clear ();
> -  }
> -
>   template <typename Base>
>   bool
>   ]b4_parser_class[::basic_symbol<Base>::empty () const YY_NOEXCEPT
> diff --git a/tests/headers.at b/tests/headers.at
> index 32c8d856..a58074fe 100644
> --- a/tests/headers.at
> +++ b/tests/headers.at
> @@ -125,7 +125,7 @@ AT_BISON_OPTION_PUSHDEFS([$1])
> AT_DATA_GRAMMAR([input.y],
> [[$1
> %define parse.error verbose
> -]AT_VARIANT_IF([], [%union {int integer;}])[
> +]AT_VARIANT_IF([%token <int> 'x'], [%union {int integer;}])[
> %code {
> ]AT_PUSH_IF([[
> #if defined __GNUC__ && 7 == __GNUC__
>