Re: [lmi] Cating doubles to enums

[Greg Chicares]

On 2018-11-05 22:42, Vadim Zeitlin wrote:
> On Mon, 5 Nov 2018 22:31:57 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> On 2018-11-04 20:36, Vadim Zeitlin wrote:
> GC> > On Sun, 4 Nov 2018 17:49:17 +0000 Greg Chicares <address@hidden> wrote:
> GC> > 
> GC> > GC> Therefore, what do you think of, say,
> GC> > GC>   double query    (database_key);
> GC> > GC>   void   query_ref(T&, database_key);
> GC> > GC>   void   query_ref(vector<T>&, database_key);
> GC> > GC> which distinguishes the names without denying the similarity of
> GC> > GC> the operations?
> GC> > 
> GC> >  I'm fine with this.
> GC> > 
> GC> > GC> I tried different second lexemes:
> GC> > GC>   query_value  meaningless--they all retrieve values
> GC> > GC>   query_into   seems obscure
> GC> > 
> GC> >  But my preferred version would be this one.
> GC> 
> GC> Then its name shall be called query_into<>(),
> 
>  Excellent, thank you.
> 
> GC> and the implementation for scalars is:
> GC> 
> GC> template<typename T>
> GC> void product_database::query_into(T& dst, e_database_key k) const
> GC> {
> GC>     double d = [result of calling a more fundamental overload]
> GC>     if constexpr(std::is_enum_v<T>)
> GC>         {
> GC>         dst = static_cast<T>(bourn_cast<std::underlying_type_t<T>>(d));
> GC>         }
> GC>     else
> GC>         {
> GC>         static_assert(std::is_integral_v<T>);
> GC>         dst = bourn_cast<T>(d);
> GC>         }
> GC> }
> GC> 
> GC> which falls just short of my threshold for adding a unit test module.
> 
>  But it's so short that it definitely falls under my threshold for the code
> I feel comfortable reviewing and so I did read it and have just one small
> question: why do we need this static_assert<> above?

Indeed. I should have slept on this again before publishing it.
Expunging the static_assertion is an improvement (which I'll make),
because all we should care about at that point is whether double
is bourn-castable to T, and bourn_cast already complains if it isn't.

> It looks like it could
> work without problems for float (which we'll probably never use) and for
> double, which we definitely do use, and I'm not sure if there is any point
> in forbidding using this version for double values, e.g. I can, in
> principle, see when it might be convenient to use it just for consistency.

I hadn't yet considered whether we'd want this syntax for actual
doubles. We probably will, for consistency.

For now, I've tried to cover all the floating-to-enum conversions
so that an msvc build will work again. Let me commit that now and
ask you to make sure that goal has been achieved.

Most of the changes are clearly for the better IMO, but this one
bothers me:

-    oenum_alb_or_anb const alb_anb =
-        static_cast<oenum_alb_or_anb>
-            (database_->Query(DB_AgeLastOrNearest)
-            );
+    oenum_alb_or_anb alb_anb;
+    database_->query_into(alb_anb, DB_AgeLastOrNearest);

because there's no 'alb_anb' data member to assign into; maybe
that suggests that such a member should exist, or maybe it means
I should reverse the order of the function template arguments
and default the "destination" argument:

 template<typename T>
-void product_database::query_into(T& dst, e_database_key k) const
+T product_database::query_into(e_database_key k, T& dst = T(0)) const

to allow some syntax like

-    oenum_alb_or_anb alb_anb;
-    database_->query_into(alb_anb, DB_AgeLastOrNearest);
+    auto const alb_anb = 
{database_->query_into<oenum_alb_or_anb>(DB_AgeLastOrNearest)};

Of course, a similar argument-order swap would be wanted here too:
-    void Query(std::vector<double>&, e_database_key) const;
+    void query_into(e_database_key, std::vector<double>&) const;