Re: -Wweak-vtables warnings (was: Bison 3.2.91 released [beta])

[Derek Clegg]

Super — thanks, Akim!

Derek

> On Jan 20, 2019, at 9:33 AM, Akim Demaille <address@hidden> wrote:
> 
> Hi!
> 
>> Le 20 janv. 2019 à 16:02, Derek Clegg <address@hidden> a écrit :
>> 
>> Hi, Akim —
>> 
>> On Jan 19, 2019, at 11:49 PM, Akim Demaille <address@hidden> wrote:
>>> 
>>> I don't think I will address this warning, the cure is way worse than the 
>>> problem.
>>> 
>>> To add the destructor, I must make the signature explicit, and it cannot be 
>>> different from the one in the base class, so I must add noexcept:
>>> 
>>> ~syntax_error () YY_NOEXCEPT;
>> 
>> In C++98 the signature is
>> virtual ~exception() throw();
>> and in C++11 the signature is
>> virtual ~exception();
>> so this only has to be handled specially for C++98.
> 
> Wow...  You are right!  I would never have believed they actually removed the 
> 'throw()', instead of making it 'noexcept'...
> 
> This is what I have in my copy of the C++98 standard:
> 
>> 18.6.1 Class exception   [lib.exception]
>> 
>>    namespace std {
>>      class exception {
>>      public:
>> 
>>        exception() throw();
>>        exception(const exception&) throw();
>>        exception& operator=(const exception&) throw();
>>        virtual ~exception() throw();
>>        virtual const char* what() const throw();
>> 
>> }; }
> 
> 
> and this is n3797, a draft of C++14.  I guess they did the same for C++11.
> 
>> 18.8.1 Class exception   [exception]
>> 
>>  namespace std {
>>    class exception {
>>    public:
>> 
>>      exception() noexcept;
>>      exception(const exception&) noexcept;
>>      exception& operator=(const exception&) noexcept;
>>      virtual ~exception();
>>      virtual const char* what() const noexcept;
>> 
>> }; }
> 
> 
> The reason I did not even try is that I fell onto this in libc++ while 
> addressing your bug report:
> 
> In comment:
>> class exception
>> {
>> public:
>>    exception() noexcept;
>>    exception(const exception&) noexcept;
>>    exception& operator=(const exception&) noexcept;
>>    virtual ~exception() noexcept;
>>    virtual const char* what() const noexcept;
>> };
> 
> Or actual code:
> 
>> class _LIBCPP_EXCEPTION_ABI runtime_error
>>    : public exception
>> {
>> private:
>>    _VSTD::__libcpp_refstring __imp_;
>> public:
>>    explicit runtime_error(const string&);
>>    explicit runtime_error(const char*);
>> 
>>    runtime_error(const runtime_error&) _NOEXCEPT;
>>    runtime_error& operator=(const runtime_error&) _NOEXCEPT;
>> 
>>    virtual ~runtime_error() _NOEXCEPT;
>> 
>>    virtual const char* what() const _NOEXCEPT;
>> };
> 
> with
> 
>> #ifndef _LIBCPP_HAS_NO_NOEXCEPT
>> #  define _NOEXCEPT noexcept
>> #  define _NOEXCEPT_(x) noexcept(x)
>> #else
>> #  define _NOEXCEPT throw()
>> #  define _NOEXCEPT_(x)
>> #endif
> 
> Besides, cppreference shows nothing about exception specifiers 
> (https://en.cppreference.com/w/cpp/error/exception/%7Eexception).
> 
>> Wouldn’t it be sufficient to explicitly write
>> 
>> #if 201103L <= YY_CPLUSPLUS
>> ~syntax_error();
>> #else
>> ~syntax_error() throw();
>> #endif
>> 
>> and similarly for the definition?
> 
> Yes, it does.  Actually, all this is a red-herring, ~syntax_error works fine 
> with throw() and noexcept.  It's the other uses of YY_NOEXCEPT as throw() 
> that is a problem.  I don't feel like investigating right now, but I should 
> do it at some point.
> 
>> It seems problematic to redefine YY_NOEXCEPT, especially since “noexcept” 
>> and “throw()” aren’t used equivalently.
> 
> Well, I thought it was the case where I used it.
> 
> I will install the following commit when it's validated by the CI.  It 
> depends on another commit (glr.cc: be more alike lalr1.cc).  You can fetch it 
> from my branch Wweak-vtables on GitHub.
> 
> Again, thanks a lot Derek!
> 
> 
> 
> commit 9d792e20f4e3b03226095d9b3389038bf6c14791
> Author: Akim Demaille <address@hidden>
> Date:   Sun Jan 20 08:23:41 2019 +0100
> 
>    c++: address -Wweak-vtables warnings
> 
>    Reported by Derek Clegg
>    http://lists.gnu.org/archive/html/bug-bison/2019-01/msg00021.html
> 
>    To avoid this warning, we need syntax_error to have a virtual function
>    defined in a compilation unit.  Let it be the destructor.  To comply
>    with C++98, this dtor should be 'throw()'.  Merely making YY_NOEXCEPT
>    be 'throw()' in C++98 triggers
>    errors (http://lists.gnu.org/archive/html/bug-bison/2019-01/msg00022.html),
>    so let's introduce YY_NOTHROW and flag only ~syntax_error with it.
> 
>    Also, since we add an explicit dtor, we now need to provide an
>    explicit copy ctor.
> 
>    * configure.ac (warn_cxx): Add -Wweak-vtables.
>    * data/skeletons/c++.m4 (YY_NOTHROW): New.
>    (syntax_error): Declare the dtor, and define the copy ctor.
>    * data/skeletons/glr.cc, data/skeletons/lalr1.cc (~syntax_error):
>    Define.
> 
> diff --git a/configure.ac b/configure.ac
> index a3a471af..1b5343e7 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -100,7 +100,7 @@ if test "$enable_gcc_warnings" = yes; then
>     -Wpointer-arith -Wshadow
>     -Wwrite-strings'
>   warn_c='-Wbad-function-cast -Wstrict-prototypes'
> -  warn_cxx='-Wextra-semi -Wnoexcept -Wundefined-func-template'
> +  warn_cxx='-Wextra-semi -Wnoexcept -Wundefined-func-template -Wweak-vtables'
>   # Warnings for the test suite only.
>   #
>   # -fno-color-diagnostics: Clang's use of colors in the error
> diff --git a/data/skeletons/c++.m4 b/data/skeletons/c++.m4
> index f6f95c4c..acc35b62 100644
> --- a/data/skeletons/c++.m4
> +++ b/data/skeletons/c++.m4
> @@ -77,8 +77,10 @@ m4_define([b4_cxx_portability],
> // Support noexcept when possible.
> #if 201103L <= YY_CPLUSPLUS
> # define YY_NOEXCEPT noexcept
> +# define YY_NOTHROW
> #else
> # define YY_NOEXCEPT
> +# define YY_NOTHROW throw ()
> #endif
> 
> // Support noexcept when possible.
> @@ -217,7 +219,14 @@ m4_define([b4_public_types_declare],
>       syntax_error (]b4_locations_if([const location_type& l, ])[const 
> std::string& m)
>         : std::runtime_error (m)]b4_locations_if([
>         , location (l)])[
> -      {}]b4_locations_if([
> +      {}
> +
> +      syntax_error (const syntax_error& s)
> +        : std::runtime_error (s.what ())]b4_locations_if([
> +        , location (s.location)])[
> +      {}
> +
> +      ~syntax_error () YY_NOEXCEPT YY_NOTHROW;]b4_locations_if([
> 
>       location_type location;])[
>     };
> diff --git a/data/skeletons/glr.cc b/data/skeletons/glr.cc
> index 778c65b8..56217341 100644
> --- a/data/skeletons/glr.cc
> +++ b/data/skeletons/glr.cc
> @@ -160,6 +160,9 @@ m4_pushdef([b4_parse_param], 
> m4_defn([b4_parse_param_orig]))dnl
>   ]b4_parser_class::~b4_parser_class[ ()
>   {}
> 
> +  ]b4_parser_class[::syntax_error::~syntax_error () YY_NOEXCEPT YY_NOTHROW
> +  {}
> +
>   int
>   ]b4_parser_class[::operator() ()
>   {
> diff --git a/data/skeletons/lalr1.cc b/data/skeletons/lalr1.cc
> index 053ba3be..1dc2851d 100644
> --- a/data/skeletons/lalr1.cc
> +++ b/data/skeletons/lalr1.cc
> @@ -562,6 +562,8 @@ m4_if(b4_prefix, [yy], [],
>   ]b4_parser_class::~b4_parser_class[ ()
>   {}
> 
> +  ]b4_parser_class[::syntax_error::~syntax_error () YY_NOEXCEPT YY_NOTHROW
> +  {}
> 
>   /*---------------.
>   | Symbol types.  |
>