Re: [lmi] [PATCH] Remove unnecessary definitions of pure virtual functions

[Vadim Zeitlin]

On Wed, 15 Mar 2017 16:25:17 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2017-03-15 12:59, Vadim Zeitlin wrote:
GC> > On Wed, 15 Mar 2017 10:29:31 +0000 Greg Chicares <address@hidden> wrote:
GC> > 
GC> > GC> Interestingly, though, this example shows that, given Base::foo()=0, 
if any
GC> > GC> Derived::foo() returns, then an implementation of Base::foo() cannot 
be
GC> > GC> marked [[noreturn]] even if it cannot return...
GC> > 
GC> >  Sorry, why do you think this? Base::foo() could be marked as 
[[noreturn]],
GC> > according to my reading of the Standard (although, admittedly, this is
GC> > another of those not totally clear things about the attributes...), the
GC> > attribute only applies to the Base method and is not inherited by the
GC> > derived classes. So, in principle, this is possible.
GC> 
GC> Studying it pretty carefully a few hours ago didn't necessarily leave me
GC> with a strong impression either way. I was assuming it was an error, like:
GC>   Base virtual int foo() const;
GC>   Derived   double foo() override;
GC> but that was only an assumption without demonstrable grounds.

 I could be wrong, of course, but, again, I can at least confirm that, in
practice, using [[noreturn]] on the base class version, but not in the
derived class does work with the 3 compilers.


GC> >  Notice that MSVC gives a warning about using [[noreturn]] with a non-void
GC> > function, so if we ever need to do something like this, we'd need to
GC> > suppress this warning for it. But it's "just" a warning and, in fact, I 
had
GC> > started by doing it like this before realizing that these pure virtual
GC> > methods definitions could be just removed and it did work (although I
GC> > didn't test that patch with gcc).
GC> 
GC> I consider this warning a mistake. If it can't conveniently be suppressed
GC> with a specific compiler flag (without forcing us to litter our code with
GC> pragmas), then I call it a compiler defect.

 It can be suppressed globally, of course, just as any other warning. But I
think it's a useful warning because I think there are very few situations
in which using [[noreturn]] with non-void functions is done intentionally
and is not a mistake.


GC> The crux of that argument is that "a non-void return value ... implies that
GC> the function returns a value to the caller". If that's true, then this
GC> declaration
GC>   const_reference std::vector::at(size_type n) const;
GC> implies that at() returns a value, which it cannot do if it happens to 
throw.

 Sorry, I have to disagree with this "if ... then". The two situations are
different, a [[noreturn]] function guarantees that it *never* returns
anything while std::vector::at() may only *sometimes* fail to return
something. The first statement is not just quantitatively much stronger,
but qualitatively different.

GC> This argument is erroneous because it conflates
GC>  - the type of the return value, with
GC>  - whether the function returns at all
GC> but those matters are orthogonal.

 No, logically, they are not at all. For me it doesn't make sense to speak
of a type of the value which is never returned because it could be
literally anything at all, i.e. "[[noreturn]] T1 foo()" and "[[noreturn]]
T2 foo()" are completely equivalent even if T1 and T2 are very different
types. So in order to give the function return type a well-defined meaning
we'd better assume that the function is not [[noreturn]] in the first
place.


GC> Consider this slightly different circumstance:
GC> 
GC>   virtual int Base::foo();
GC> 
GC>   [[noreturn]] Derived::foo()
GC>   {
GC>     throw "Just a stub for now--to be implemented later."
GC>   }

 If foo() is indeed not pure (as written), Derived::foo() shouldn't be
defined at all. If foo() was meant to be pure, then this code is not
finished because a pure virtual function must be really implemented, not
just stubbed out -- i.e. this code will fail during run-time when (not
"if") it's called. This doesn't mean that the code can't be left in this
state for some time, but, again, this is not its final, or satisfactory,
form and it's the least of things to get a warning (either from gcc
-Wsuggest-attribute=noreturn or from MSVC about using [[noreturn]] with a
non-void function) for it. I.e. I actively do want to have a warning here
to remind me about the problem.

GC> I think the case for non-void noreturn is even stronger in the case of lmi's
GC> 'round_to.hpp':
GC> 
GC>   template<typename RealType>
GC>   class round_to {
GC>   ...
GC>     rounding_fn_t rounding_function_ {detail::erroneous_rounding_function};
GC> 
GC> Nondefault ctors assign a particular function to the rounding_function_
GC> pointer. The default ctor initializes that function pointer with a function
GC> that deliberately throws an informative error message. cert.org says that
GC> this "can result in misuse of the API by the consumer", but I would say it
GC> guards against misuse.
GC> 
GC> Does a preferable alternative exist?

 IMO the most preferable alternative by far would be to forbid constructing
the object in an invalid state, IME such objects are a huge source of
problems (if NULL was Hoare's billion-dollar mistake, then invalid objects
are at least a 500-million-dollar one). By allowing this state to exist, we
force ourselves to subvert the C++ type system (not that it's very
difficult, of course...) because the object needs to have a rounding_fn_t
function and can't have it at the same time. This is the real source of the
problem and the only solution to it provided by C++ is to use pure virtual
methods which would indeed be quite inconvenient to use here.

 Regards,
VZ