Re: [lmi] Cating doubles to enums

[Vadim Zeitlin]

On Fri, 2 Nov 2018 16:37:51 +0000 Greg Chicares <address@hidden> wrote:

GC> That set of files sounds like an exact match for
GC>   "static_cast directly from double to enum"
GC>   git log b48b07fd --stat

 Yes, sorry, I should have mentioned this.

GC> >  Somewhat to my surprise, I confirmed that casting floating point values 
to
GC> > (complete) enum values is indeed valid in C++ but nevertheless MSVS seems
GC> > to intentionally disallow this.
GC> 
GC> I believe I've followed the standard, as quoted in the commit message:
GC> 
GC> | A value of floating-point type can also be explicitly converted to an
GC> | enumeration type.
GC> 
GC> so this seems to be plainly a defect in their compiler.

 Yes, as I said I was surprised to find that casting floating point values
to enums using static_cast<> was allowed (I honestly never thought of doing
this before), but it is, quite explicitly.

GC> I don't think these lines of code change often. And I have the
GC> impression that MS have begun to take standard compliance more
GC> seriously than they once did, so they'll presumably fix this.

 I hope so, but in the meanwhile this bug report

https://developercommunity.visualstudio.com/content/problem/154236/error-c2440-when-casting-double-to-enum.html

is open since almost a year without any real feedback so far, so it doesn't
seem like it's going to happen very soon.

GC> But meanwhile, it's an actual problem for you, and I suppose we
GC> could temporarily revert that commit if there's no other way.
GC> First of all, though, is there some #pragma we could use to
GC> disable it, preferably on a global basis (say, OAOO, in a
GC> PCH header)?

 Unfortunately, I don't think so. It's an error, not a warning, and so is
not subject to any #pragmas.

GC> >  So I wonder if you would consider adding some kind of enum_cast<> 
function
GC> > which would cast a double to int first using bourn_cast<> and then,
GC> > possibly after checking the validity of the int as an enum element, cast 
it
GC> > to the target type using static_cast<>? This would solve my problem, which
GC> > is the immediate reason for proposing it, but would also IMO be slightly
GC> > more readable and potentially safer.
GC> I'm guessing that b48b07fd is where this arose; and it looks like
GC> every line changed by that commit contains a call to Query(); so
GC> I think the proper solution is to "overload" Query().

 Ah, indeed, I haven't looked at this in sufficient detail, as usual, but
the problem seems to be indeed localized at Query() level and fixing it
there would seem to be even better.

GC> "Overload" is probably the wrong word, because what I have in mind is
GC> adding a template argument. 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.

 To summarize, my preferences would be:

1. template<typename T> T Query(e_database_key)
2. template<typename T> void GetValueOf(T&, e_database_key)
...
9. template<typename T> void Query(T&, e_database_key)

[end of nitpicking]

GC> and ultimately, because Query() is probably always invoked on
GC> BasicValues::Database_,
GC> 
GC> +   Query(WaiverChargeMethod, DB_WpChargeMethod);
GC> 
GC> for a new member function BasicValues::Query(...) that would forward
GC> to product_database::Query(...), which would have to become, e.g.:
GC> 
GC>      double Query(e_database_key) const;
GC>      double Query(e_database_key, database_index const&) const;
GC> +    template<typename T>
GC> +    void Query(T& t, e_database_key) const;
GC>      void Query(std::vector<double>&, e_database_key) const;
GC>      void Query(std::vector<double>&, e_database_key, database_index 
const&) const;
GC> 
GC> where the new function would, as you suggest, first bourn_cast
GC> from double for safety, and then...by the most appropriate method,
GC> which is not immediately apparent to me...cast to the right enum type.
GC> And if MSVS fails to support the most appropriate standard method,
GC> then a workaround in one single place (the Query<>() implementation)
GC> wouldn't be too terrible.
GC> 
GC> Sound good?

 Yes, absolutely. Do you prefer to do it yourself or would you like me to
do this? This not very urgent for me, but I do want to fix MSVS build
sooner rather than later. I could, of course, just revert b48b07fd for now
in my branch, but this means that I'll have a bunch of conflicts when I
merge the changes from master into it later (which is still not really a
big problem, of course, but if I can avoid it, I'd still prefer it).

 Please let me know,
VZ