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

[Greg Chicares]

On 2017-03-15 16:51, Vadim Zeitlin wrote:
> On Wed, 15 Mar 2017 16:25:17 +0000 Greg Chicares <address@hidden> wrote:
[...]
> 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.

I would nevertheless maintain that class round_to, as it stands, chooses
the option that is least bad, or at least no worse than any other, if we
quite reasonably want to make this class default-constructible--e.g., so
that we can do
  std::vector<round_to> RoundFns;
  std::copy
    (std::istream_iterator<round_to>(cin)
    ,std::istream_iterator<round_to>(),
    back_inserter(RoundFns)
    );
or even so that we can just compile lmi as it stands, given:

  class LMI_SO BasicValues{...
    round_to<double> round_specamt_;
  ...};

  void BasicValues::SetRoundingFunctors()
  {...
    set_rounding_rule(round_specamt_, RoundingRules_->datum("RoundSpecAmt"));
  ...}

Should we give up the ability to hold a round_to object as a member? or the
ability to initialize it after the proper context has been set (such as the
name of the file from which the rules are read)?

We could initialize rounding_function_ to a "common" choice by default:
detail::round_near(), for example. But that introduces the possibility of
using a round_to object that should have been initialized otherwise, and
the erroneous omission will go undetected. That's less bad than initializing
it to nullptr, which could cause a crash. But default-initializing it as at
present makes the omission self-detecting, which, even if not ideal, is
not clearly worse than the other options.

Alternatively, if we want to do something like
  std::vector<round_to>(9)
but we aren't willing to make the class DefaultConstructible, then of course
we could resort to
  std::vector<std::shared_ptr<round_to>>
but I would argue that std::shared_ptr's default ctor returns an object
in an invalid state. It could be argued that a pointer that must not be
dereferenced is "valid", and that only the act of dereferencing it is
actually invalid; but by the same token I could retort that a default-
constructed round_to is "valid" in the same "unless you use it" sense.
There's a difference, though: as above, throwing an exception is not worse
than crashing.

Consider again the BasicValues code above. If (as a matter of corporate
policy, say) we had to conform to every securecoding.cert.org rule, then
lmi would be deemed "insecure", and we'd be compelled to fix it...and we
would naturally fix this "violation" by eradicating an appropriate use of
"[[noreturn]]", even though that would make lmi no better and obviously
no more "secure". As a "suggestion to consider" or a "guideline to follow
unless there's a good reason not to", it's okay. But they mandate what we
"must" do, raise the specter of "undefined behavior", speak of "Noncompliant
Code", present a "Risk Assessment", and call this a "vulnerability"; that's
uncalled for and not even correct, and it weakens the credibility of the
legitimate, reasonable guidance they ordinarily publish.