Re: [lmi] Stream cast problems with trailing spaces and more

[Greg Chicares]

On 2020-01-29 12:24, Vadim Zeitlin wrote:
> On Wed, 29 Jan 2020 02:15:53 +0000 Greg Chicares <address@hidden> wrote:
[...]
> GC> On 2018-11-16 01:00, Vadim Zeitlin wrote:
> GC> > 
> GC> >  I've been looking at stream_cast() code because it started throwing
> GC> > "Trailing whitespace remains" exception with the latest MSVS version
> GC> > (15.9.0) when initializing Input::ListBillDate field from its JDN given 
> in
> GC> > the string form in this class ctor, which means that no illustrations 
> can
> GC> > be created when using lmi built this compiler, which is, as you can
> GC> > imagine, somewhat problematic. Leaving aside the question of why does 
> this
> GC> > member need to be initialized from a string in the first place,
> GC> 
> GC> Initialization from string for uniformity:
> GC> 
> GC> Input::Input()
> GC>     :IssueAge                         {"45"}
> GC>     ,RetirementAge                    {"65"}
> GC>     ,Gender                           {"Male"}
> GC> ...
> GC>     ,ListBillDate                     {"2440588"} // Assume no inforce so 
> old
> 
>  I guess the question could be formulated as "why is uniformity important
> here?". I don't really like specifying numbers (or dates) as strings and I
> don't see why do we need to convert strings to their actual type during
> run-time instead of doing it during compile-time. I admit I haven't done
> any profiling here, but I think Input objects are created relatively often,
> so using numbers could be not only simpler (fewer quotes!), but also
> faster.

I measure an Input::Input() speedup of a few microseconds, less than a percent:
  http://git.savannah.nongnu.org/cgit/lmi.git/commit/?h=odd/eraseme_input_ctor
When differences are so slight, we might very well be measuring random noise.

Input objects are indeed created relatively often. And their initialization
is costly enough to matter. But initializing their scalar members is only a
tiny part of the cost. I wouldn't be surprised if initializing a single one
of the many input-sequence members costs more than initializing all of the
scalar members.

I don't feel sure that a patch like this can introduce no subtle defect. It's
unwise to accept any slight risk in return for a negligible speed increase.

>  BTW, now that we use C++17, I'd seriously consider defining a couple of
> custom literal operators for calendar_date, that would allow us to write
> 2440588_jdn or even 19700101_ymd instead of calendar_date(jdn_t(2440588)).

I think user-defined literals are like specialized kitchen gadgets--e.g.,
cherry-pitters or strawberry-hullers. If you need to remove the pits from
ten kilograms of cherries, a specialized tool might be a good investment.
But for ten grams of cherries, the benefit isn't enough to justify the
cost of learning to use it. I think our time is better spent elsewhere
in this case.

> GC> Please take a look at how I changed that (once I push the commit).
> 
>  I'll have a look at the commit once it's done and will test it with clang
> and MSVC.

Thanks--I'm not using those tools, so I might have again make a mistake
that's not readily visible with gcc.

> GC> >  Finally, while I'm looking at this code, I can't help wondering why do 
> we
> GC> > have a dummy "To" parameter in this function template? Is it some
> GC> > workaround for ancient and not supported any more compiler(s)? Because 
> it
> GC> > doesn't seem to be needed any longer.
> GC> 
> GC> Isn't it needed because the template-parameter list and the argument list
> GC> have different orders?
> 
>  Sorry, I might be missing something obvious here, but I don't see how can
> this be a problem? My point was, I think, that we can, and should, call
> stream_cast<> with explicit To, just for any other "casts", i.e. write
> 
>       To to = stream_cast<To>(from);
> 
> instead of
> 
>       To to;
>       to = stream_cast(from, to);
> 
> which is currently allowed (and used in a couple of places), but seems
> somewhat confusing to me, because it's natural to write
> 
>       To to;
>       stream_cast(from, to);
> 
> expecting that this would modify "to".

Maybe. But OTOH the existing code is valid AFAICS, so why would we
change stream_cast<> in a way that makes it invalid?

[snip example of errors that would ensue]

>  This template specialization would need to be adjusted and its
> "std::string" dummy parameter removed, of course.
> 
>  If you'd like, I could make (and test) a patch removing the "To = To()"
> dummy template parameter to show more precisely what I suggest to do (once
> you commit your modifications to stream_cast<>, to avoid any conflicts).

I've pushed my commits, so go ahead and show exactly what you have
in mind, if you like. When I tried the simplest change I could think
of along these lines (above), things broke; but maybe this really is
a worthwhile change and I simply haven't understood the idea.