Re: [lmi] Cating doubles to enums

[Greg Chicares]

On 2018-11-02 16:56, Vadim Zeitlin wrote:
> On Fri, 2 Nov 2018 16:37:51 +0000 Greg Chicares <address@hidden> wrote:
> GC> [...] Given...
> GC> 
> GC>     oenum_waiver_charge_method   WaiverChargeMethod;
> GC> 
> GC> improve this b48b07fd change...
> GC> 
> GC> -    WaiverChargeMethod  = 
> static_cast<oenum_waiver_charge_method>(static_cast<int>(Database_->Query(DB_WpChargeMethod)));
> GC> +    WaiverChargeMethod  = 
> static_cast<oenum_waiver_charge_method>(Database_->Query(DB_WpChargeMethod));
> GC> 
> GC> ...thus:
> GC> 
> GC> -   WaiverChargeMethod  = 
> static_cast<oenum_waiver_charge_method>(Database_->Query(DB_WpChargeMethod));
> GC> +   Database_->Query(WaiverChargeMethod, DB_WpChargeMethod);
> 
>  Sorry for nitpicking but I think that the best version would be
> 
>       WaiverChargeMethod = 
> Database_->Query<oenum_waiver_charge_method>(DB_WpChargeMethod);
> 
> IMO the more natural syntax is worth explicitly specifying the variable
> type. I already find the existing Query(vector<double>&, ...) overloads
> very confusing and I'd like to avoid making the scalar Query() similar to
> them. If possible (but this, of course, goes way beyond fixing MSVS build),
> I'd either change or maybe at least rename the vector ones.
> 
>  In fact, renaming might be a solution combining maximal brevity with
> decent readability, e.g.
> 
>       Database_->GetValueOf(WaiverChargeMethod, DB_WpChargeMethod);
> 
> doesn't seem too bad. But Query() is really supposed to return the result
> of its query and seeing an expression involving Query() but not assigning
> its return value to anything just looks very suspicious to me.

Okay, there are multiple issues here:

(1) Too-diverse overloading of the single name Query(). It already has
  (a) a simple form that takes one scalar argument and returns a scalar
  (b) a vector form that's substantially different
  (c) a couple others for exotic purposes
and I had proposed adding yet another form.

(2) Brevity: 'WaiverChargeMethod' is of type 'oenum_waiver_charge_method',
and of these candidates:

   WaiverChargeMethod  = 
static_cast<oenum_waiver_charge_method>(Database_->Query(DB_WpChargeMethod));
   WaiverChargeMethod = 
Database_->Query<oenum_waiver_charge_method>(DB_WpChargeMethod);
   Database_->GetValueOf(WaiverChargeMethod, DB_WpChargeMethod);
   Database_->Query(WaiverChargeMethod, DB_WpChargeMethod);

the last two are much more concise, and more in the spirit of modern C++,
much like not repeating a vector's type here:
-  std::vector<T>::const_iterator i = v.begin();
+  auto i = v.begin();

Contrasting the last two candidates above:

- I agree that the syntactic difference is great enough to call for
  a lexical distinction; but

- I balk at making the name radically different. 'Query' is pretty
  much the only useful database operation in client code: it's like
  operator() with a non-elided name. The present two overloads, and
  the proposed third, all dispatch queries after all: they're
  different porcelain on the same plumbing.

Therefore, what do you think of, say,
  double query    (database_key);
  void   query_ref(T&, database_key);
  void   query_ref(vector<T>&, database_key);
which distinguishes the names without denying the similarity of
the operations? I tried different second lexemes:
  query_value  meaningless--they all retrieve values
  query_into   seems obscure
  query_assign dunno--maybe
  query_imbue  perhaps
but couldn't find one that's clearly better than 'query_ref'
(although query_imbue is starting to grow on me).

Or maybe the last two are distinct enough operations to deserve
different names, because a T& and a vector<T>& are so different that
calling them both '_ref' isn't optimally clear...so:
  double query      (database_key);
  void   query_imbue(T&, database_key);
  void   query_fill (vector<T>&, database_key);

> GC> Sound good?
> 
>  Yes, absolutely. Do you prefer to do it yourself
Yes, I hope to get it finished today.

[I had first thought of doing this when trying to eliminate shared_ptr
members of class BasicValues (which, BTW, are defectively non-const).
For the product_database shared_ptr member, when I replaced
'pointer->' with 'accessor()->' throughout clients of BasicValues,
I noticed that almost all instances were calls to Query(), e.g.:

-    TermCanLapse                = Database_->Query(DB_TermCanLapse);
+    TermCanLapse                = database().Query(DB_TermCanLapse);

I had to put that work aside in order to get a working version of a
new "death benefit option", but now that I've done that, I'm going
back and committing this work that I had put aside, lest it grow too
stale. That's why I'm addressing Query() soon, but not immediately.]