[lmi] value_cast() with two arguments [Was: Unit tests hygiene]

[Greg Chicares]

On 6/3/22 17:47, Vadim Zeitlin wrote:
> On Fri, 3 Jun 2022 16:29:18 +0000 Greg Chicares <gchicares@sbcglobal.net> 
> wrote:
> 
> GC> On 5/21/22 18:34, Vadim Zeitlin wrote:
[...]
> GC> > - Or we could change "To" in value_cast() to "To const&" to ensure that 
> the
> GC> >   value never needs to be accessed.
> GC> 
> GC> Can you readily state the location of the offending value_cast()?
> 
>  Yes, it's the any_member.hpp line mentioned above, i.e. 177.
> 
>  Just to be totally clear: we _copy_ the value of the not yet initialized
> mec_state::C3_init_is_mc while we're initializing it. Copying it involves
> reading it which, formally, counts as accessing uninitialized memory. By
> not copying it here, this access would be avoided.
> 
>  In fact, thinking more about this, I really believe that we should
> implement not one or the other suggestion, but both of them. Default
> initializing is good and passing objects by value is bad, so let's do the
> former and not do the latter even if it doesn't really change anything
> much because why not?

I had been putting this off because it didn't seem urgent, but did
seem to require deep thought. But now I've looked into it, and it
seems to be well worth doing.

My first thought was to write a failing unit test that duplicates
the reported problem, but that seems to be impossible: even if we
ask the compiler to give us an illegally-initialized bool, that's
UB and it can do anything it pleases, such as giving us 'true'.

But before making such a change, I'd like at least to know where
this overload is called, so I applied this experimental change:

--8<----8<----8<----8<----8<----8<----8<----8<----8<----8<----8<--
diff --git a/value_cast.hpp b/value_cast.hpp
index 5c237a6db..a81d73e27 100644
--- a/value_cast.hpp
+++ b/value_cast.hpp
@@ -235,11 +235,11 @@ To value_cast(From const& from)
     static_assert(!std::is_pointer_v<To>);
     return value_cast_chooser<To,From>()(from);
 }
-
+#if 0
 template<typename To, typename From>
-To value_cast(From const& from, To)
+To value_cast(From const& from, To const&)
 {
     return value_cast<To,From>(from);
 }
-
+#endif // 0
 #endif // value_cast_hpp
--8<----8<----8<----8<----8<----8<----8<----8<----8<----8<----8<--

and built lmi, filtering the error messages:

/opt/lmi/src/lmi[0]$make LMI_COMPILER=clang LMI_TRIPLET=x86_64-pc-linux-gnu 
--output-sync=recurse $coefficiency install check_physical_closure 2>&1 | grep 
'error:' | sort -u
/opt/lmi/src/lmi/any_member.hpp:177:23: error: no matching function for call to 
'value_cast'

/opt/lmi/src/lmi[0]$make LMI_COMPILER=gcc LMI_TRIPLET=x86_64-pc-linux-gnu 
$coefficiency install check_physical_closure 2>&1 | grep 'error:' | sort -u 
2>&1 |less -S 

[Note: This demonstrates an advantage of working with more than one
compiler. The 'clang' command above emits only one line, which tells
us exactly what we want to know; but the output gets scrmabled unless
we specify '--output-sync=recurse'. The 'gcc' command spat out TMI
for this line of inquiry:

  /opt/lmi/src/lmi/any_member.hpp:177:33: error: no matching function for call 
to ‘value_cast(const string&, bool&)’
  /opt/lmi/src/lmi/any_member.hpp:177:33: error: no matching function for call 
to ‘value_cast(const string&, ce_product_name&)’
  /opt/lmi/src/lmi/any_member.hpp:177:33: error: no matching function for call 
to ‘value_cast(const string&, ce_skin_name&)’
  /opt/lmi/src/lmi/any_member.hpp:177:33: error: no matching function for call 
to ‘value_cast(const string&, database_entity&)’
  ...

but didn't require '--output-sync=recurse'. --end note]

So, aside from unit tests (and antediluvian stuff that end users never
see), that single call in 'any_member.hpp':
    object_->*held_ = value_cast(s, object_->*held_);
is the only reason for this two-argument value_cast<>() overload to exist.
Thus, we should remove that overload, and replace that single line:
    object_->*held_ = value_cast(s, object_->*held_);
    object_->*held_ = value_cast<DESIRED_TYPE>(s);
and then clean up anything not covered by 'make install', e.g., the
any_member unit test.

Clearly decltype() is the tool for this job because it doesn't evaluate
its operand. This sounded easy in principle, but it took me a few tries
to come up with something that seems to work:

     LMI_ASSERT(object_);
-    object_->*held_ = value_cast(s, object_->*held_);
+    object_->*held_ = 
value_cast<std::remove_reference_t<decltype(object_->*held_)>>(s);
     return *this;

Does it really have to be that verbose? I suspect that it does:
 - the LHS of the assignment indicates the type that's wanted;
 - it's therefore what we once could simply call an lvalue;
 - for that reason, decltype() returns a reference type; and
 - we need to remove the reference, and the library provides a
   way to do that directly (std::decay<> would be overkill,
   though it would save eleven characters).

Is there any better way?

This:
  template<typename To, typename From>
  To stream_cast(From from, To = To())
should also be reworked. If 'To' is bool, then at least "To()"
isn't undefined behavior, but it's unnecessary to construct
anything (even if the compiler would elide it) and there's no
reason to restrict this to types that have a default ctor.