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

[Greg Chicares]

On 2017-03-15 12:59, Vadim Zeitlin wrote:
> On Wed, 15 Mar 2017 10:29:31 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> Interestingly, though, this example shows that, given Base::foo()=0, if 
> any
> GC> Derived::foo() returns, then an implementation of Base::foo() cannot be
> GC> marked [[noreturn]] even if it cannot return...
> 
>  Sorry, why do you think this? Base::foo() could be marked as [[noreturn]],
> according to my reading of the Standard (although, admittedly, this is
> another of those not totally clear things about the attributes...), the
> attribute only applies to the Base method and is not inherited by the
> derived classes. So, in principle, this is possible.

Studying it pretty carefully a few hours ago didn't necessarily leave me
with a strong impression either way. I was assuming it was an error, like:
  Base virtual int foo() const;
  Derived   double foo() override;
but that was only an assumption without demonstrable grounds.

>  Notice that MSVC gives a warning about using [[noreturn]] with a non-void
> function, so if we ever need to do something like this, we'd need to
> suppress this warning for it. But it's "just" a warning and, in fact, I had
> started by doing it like this before realizing that these pure virtual
> methods definitions could be just removed and it did work (although I
> didn't test that patch with gcc).

I consider this warning a mistake. If it can't conveniently be suppressed
with a specific compiler flag (without forcing us to litter our code with
pragmas), then I call it a compiler defect.

This author observes that the standard could forbid [[noreturn]] on a
function with a non-void return type, but chooses not to, and concludes:

http://stackoverflow.com/questions/38623143/can-i-use-noreturn-on-non-void-returning-functions/38623312#38623312
| If the function returns (whether void or int or vector<vector<double>>),
| then behavior is undefined. If the function doesn't return, the return type
| is immaterial.

This author at cert.org disagrees:

http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1453.htm
| It does not make sense for a noreturn function to have a return type other 
than void.

The C++ Committee didn't adopt that viewpoint, but his own organization did:

https://www.securecoding.cert.org/confluence/display/cplusplus/DCL22-CPP.+Functions+declared+with+[[noreturn]]+must+return+void
| DCL22-CPP. Functions declared with [[noreturn]] must return void
| If a function declared with the [[noreturn]] attribute has a non-void
| return value, it implies that the function returns a value to the caller
| even though it would result in undefined behavior. Therefore, functions
| declared with [[noreturn]] must also be declared as returning void.
| Risk Assessment
| A function declared with a non-void return type and declared with the
| [[noreturn]] attribute is confusing to consumers of the function because the
| two declarations are conflicting. In turn, it can result in misuse of the
| API by the consumer or can indicate an implementation bug by the producer.

The crux of that argument is that "a non-void return value ... implies that
the function returns a value to the caller". If that's true, then this
declaration
  const_reference std::vector::at(size_type n) const;
implies that at() returns a value, which it cannot do if it happens to throw.
This argument is erroneous because it conflates
 - the type of the return value, with
 - whether the function returns at all
but those matters are orthogonal.

In the particular circumstance addressed by this patch, the real reason why
we chose to remove two function definitions is that they were dead code.
It is merely incidental that this compiler warning happened to identify
dead code. If instead we had discovered this situation by running an
enhanced gcc with -Widentify-all-dead-code, then we would have removed it
because it was dead code--not because it has a non-void return type.

Consider this slightly different circumstance:

  virtual int Base::foo();

  [[noreturn]] Derived::foo()
  {
    throw "Just a stub for now--to be implemented later."
  }

If we hadn't written "[[noreturn]]", then a compiler might quite reasonably
have suggested it. What's the risk? How can that "result in misuse of the
API by the consumer" as cert.org suggests?

I think the case for non-void noreturn is even stronger in the case of lmi's
'round_to.hpp':

  template<typename RealType>
  class round_to {
  ...
    rounding_fn_t rounding_function_ {detail::erroneous_rounding_function};

Nondefault ctors assign a particular function to the rounding_function_
pointer. The default ctor initializes that function pointer with a function
that deliberately throws an informative error message. cert.org says that
this "can result in misuse of the API by the consumer", but I would say it
guards against misuse.

Does a preferable alternative exist? This function
  template<typename RealType>
  [[noreturn]]
  RealType erroneous_rounding_function(RealType)
  {
      throw std::logic_error("Erroneous rounding function.");
  }
cannot be given a void return type, because that would prevent forming a
rounding_fn_t pointer to it. The [[noreturn]] attribute seems obviously
appropriate, because it cannot return. I suppose we could initialize the
function pointer to nullptr in the default ctor instead, but in that case
we'd get undefined behavior instead of an orderly exception, and that
doesn't seem like "secure coding" to me.