Re: [lmi] Switch to using C++11 uniform initialization in the ctor initializer lists?

[Greg Chicares]

On 2018-08-23 18:20, Vadim Zeitlin wrote:
> On Thu, 23 Aug 2018 14:36:02 +0000 Greg Chicares <address@hidden> wrote:
[...]
> GC> >         https://github.com/vadz/lmi/compare/uniform-init?expand=1
> GC> 
> GC> That looks great. The only thing I'd change is whitespace, e.g.:
> GC> 
> GC> -        :image_{image}
> GC> -        ,src_{src}
> GC> -        ,scale_factor_{scale_factor}
> GC> +        :image_        {image}
> GC> +        ,src_          {src}
> GC> +        ,scale_factor_ {scale_factor}
> GC> 
> GC> to facilitate reading it as two columns.
> 
>  I'll do it, but in a separate commit as I prefer not mixing non-trivial
> and white-space-only changes together. BTW, this is not at all the only
> place where things are not aligned using this style, so I assume you'd like
> me to fix all of those.

Yes, that would be great.

> GC> - I'm working intensively on these files:
> GC>     report_table*.?pp wx_table_generator.?pp
> GC> and would prefer to modify those myself in order to avoid a merge.
> 
>  OK, I've left them out, but they both do contain member initializers using
> parentheses.

Indeed.

> GC> >  It only won't work if there is a narrowing conversion. In this case we 
> can
> GC> > still perform the conversion explicitly or try to get rid of it. And 
> either
> GC> > would be an improvement compared to an implicit conversion IMHO.
> GC> 
> GC> Any narrowing conversions might pose unexpected challenges, but
> GC> AFAICS that's only if they reveal unrecognized implicit conversions,
> GC> which may reasonably be regarded as defects anyway.
> 
>  I do get quite a few errors after switching to {}. Especially worrisome
> are those that resulted from initially incorrectly changing std::deque and
> std::vector ctors to use {} instead of (): this actually changed the code
> meaning, as with () it used the "traditional" ctor, from C++98, while with
> {} it started using ctor taking std::initializer_list.

Are there any other types of errors, aside from those especially
worrisome ones? I imagine you might find some lines like
  long double xyzzy = 0.0;
where '0.0L' would be preferable, but I can't think of anything else.

>  This raises 2 questions:
> 
> 1. How should the code using these ctors, e.g. the initialization of
>    seriatim_keywords_ and seriatim_numbers_ in input_sequence.cpp be
>    actually written? For now I'm keeping () as it seems the most robust and
>    also, arguably, the clearest things to do (the latter because it draws
>    attention to the fact that this is not just a simple initialization but
>    a call to a ctor which does something less trivial).

Yes, that sounds best. There isn't any reasonable alternative, is there?

> 2. How to ensure that I didn't miss any vectors (or deques, as there is
>    at least one of those, suffering from exactly the same problem, in
>    mc_enum.hpp) that shouldn't use {} initialization but do now, because
>    changing them didn't result in a compilation problem?

The only option is thorough review. How would you like to handle any
lines that I think are changed incorrectly? I don't want to commit a
changeset that introduces defects, and then fix them in a separate
commit: excluding them from the repository sounds better. I could
propose a patch to your patch, and then cherry-pick a revised version.

>    There are plenty
>    of std::vector<int>s in lmi code that could be affected by this. I'm
>    going to try to review their use, but this risks taking much more time
>    than I thought it would and still is error-prone.

I suspect that I've often instantiated 'std::vector v(cardinality)'
and sometimes even 'std::vector v(cardinality, default_element)'.

>  Would you have any thought about either of these points?

One idea is not to modernize container construction. I'm guessing
that 'ContainerType z(anything)' will rarely if ever be compatible
with '{}' in the lmi corpus.

> GC> > GC>     -    std::vector<int> const expected = {3, 4, 5};
> GC> > GC>     -    BOOST_TEST(widths(v) == expected);
> GC> > GC>     +    BOOST_TEST(widths(v) == std::vector<int>({3, 4, 5}));
> GC> > GC>     but writing the parentheses that the preprocessor then requires 
> is
> GC> > GC>     too high a price to pay.
> GC> > 
> GC> >  It should be possible to avoid the extra parentheses with the variadic
> GC> > macros, shouldn't it?
> GC> 
> GC> Maybe I'm missing something good, but I haven't tried to learn anything
> GC> about preprocessor improvements since approximately C89.
> 
>  Would you want me to try to change lmi testing macros to allow not using
> the parentheses? I don't know how difficult is it going to be, but
> hopefully not too much.

We're talking about macros. I would counsel against hope.

>  OTOH I must admit that I'm not very enthusiastic about writing our own
> macros when other people have already done it, and done it well. I still
> think that we should just add catch.hpp (yes, it's just a single header) to
> lmi source tree and use it instead of lmi weirdly named custom macros, as
> CATCH provides many more features (some of which are very useful) and a
> much better test running experience. Just compare
> [...snip example...]
>  I wonder if there is any chance of convincing you to use it instead of the
> current test_tools.hpp? I'd be very enthusiastic about this...

You might be unaware of the coding horrors that lurk in some of those
old unit tests. Not long ago, I rewrote the ancient input_sequence
tests, and found that I could hardly understand them. It would be
easy to replace a test that's valid and useful (but comprehensible
only with great difficulty) with one that is easy to read but checks
something rather different and less valuable. To do this well would
be extremely costly.

Besides, my test-running experience is already nearly perfect. I run
  $./nychthemeral_test.sh 2>&1 | tee /tmp/lmi/logs/log | sed -f errors.sed
often dozens of times a day, and certainly before every git-push.

Can you name some features that I might find useful? I've read
  https://github.com/catchorg/Catch2/blob/master/docs/why-catch.md#top
line by line, but found nothing to draw my interest.