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

[Akim Demaille]

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.  |