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

[Vadim Zeitlin]

On Thu, 23 Aug 2018 00:48:49 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2018-08-22 21:35, Vadim Zeitlin wrote:
GC> > 
GC> >  I now hesitate every time when initializing a member or a base class in
GC> > the ctor initializer list in lmi code as I'm not sure whether we're
GC> > supposed to use "uniform initialization" syntax for it, i.e. braces, or 
the
GC> > old-style parentheses.
GC> 
GC> I myself also hesitate. In practice, I look at 'input_sequence_parser.hpp',
GC> where we chose a style deliberately after much discussion, and just try to
GC> follow that:
GC> 
GC>   class SequenceParser final
GC>   {
GC> ...
GC>       token_type current_token_type_               {e_startup};
GC>       double current_number_                       {0.0};
GC>       std::string current_keyword_                 {};
GC>       int current_duration_scalar_                 {0};
GC> 
GC> although probably I often forget to do so and just write
GC>   foo::foo(int xyz) : xyz_(xyz)
GC> out of habit. And I'm not sure we ever did come up with a general style
GC> for this, one that answers all questions--e.g., in the
GC>   foo::foo(int xyz) : xyz_(xyz)
GC> example, should the header have something like
GC>   int xyz_ {};
GC> or
GC>   int xyz_ {0};
GC> even when a ctor initialization list would always change the value?

 This is a somewhat separate question. I am not really sure about this
myself, but I think that if a class has only a single ctor (not counting
copy/move ones), then it should be used to initialize all members and in
this case they shouldn't be initialized in their declarations. But if a
class has multiple ctors (including default and non-default ones), then I'd
initialize the members when declaring them and only initialize them in the
ctor initializer list if I want them have non-default values there.

GC> Is there some recent example in your work or mine, or some older example
GC> from anywhere in lmi, that we could go over in full detail with the goal
GC> of establishing the One True Way?

 I was making changes to ledger_pdf_generator_wx.cpp when the initial
question arose and while they're not finished yet, I hope that this file
might become the shining example of the best practices.

 Right now I don't think we have anything, which is why I had to ask this
question here.

GC>     // This move ctor is private and does not perform any escaping.
GC>     explicit text(std::string&& html)
GC>         :html_{html}
GC> 
GC> , right? So your exact question is whether I agree that
GC>         :html_{html}
GC> is perfectly fine, and needn't be changed to something like this:
GC>         :html_(html)
GC> , right? Sure, I don't see any objection to that.

 I thought we already agreed that "{html}" was fine, so the question was
rather whether you agreed that its advantage was sufficient to justify
changing the existing code and replacing all "(html)"s with "{html}".
You seem to have already answered it affirmatively below, but let me just
show what kind of changes I'm speaking about:

        https://github.com/vadz/lmi/compare/uniform-init?expand=1

This is what I've already done in an existing file, before adding another
ctor initializer, where I wanted to use {}, as I also wanted to avoid the
inconsistency with an existing initializer using ().

GC> I'm using {} a lot more these days. I'm still not sure when to use
GC> ({}) instead. To illustrate my uncertainty, compare:
GC> 
GC> $git show 4d5025d2
GC> 
GC>     Use terser syntax in unit test
GC>     
GC>     Even denser would be
GC>     -    std::vector<int> const expected = {3, 4, 5};
GC>     -    BOOST_TEST(widths(v) == expected);
GC>     +    BOOST_TEST(widths(v) == std::vector<int>({3, 4, 5}));
GC>     but writing the parentheses that the preprocessor then requires is
GC>     too high a price to pay.

 It should be possible to avoid the extra parentheses with the variadic
macros, shouldn't it? At least the following program, using my favourite
modern C++ testing framework from http://catch-lib.net/, compiles just
fine:

---------------------------------- >8 --------------------------------------
#define CATCH_CONFIG_MAIN
#include <catch.hpp>

#include <vector>

TEST_CASE("Comma without parentheses")
{
    using vec = std::vector<int>;
    vec const v{1, 2, 3};

    CHECK(v == vec{1, 2, 3});
}
---------------------------------- >8 --------------------------------------

GC> to this later commit:
GC> 
GC> $git show 27d79e25
GC> 
GC> +    // Set boolean vectors so that some columns get none.
GC> +    BOOST_TEST(std::vector<int>({0, 5, 0}) == apportion({0, 1, 0}, 5));
GC> 
GC> where '({})' didn't seem "too high a price to pay". In the former case,
GC> I thought it was a preprocessor workaround, hence distasteful. In the
GC> latter case, I thought it was a C++ language requirement, hence tasteful.

 I must be missing something, but why did you think this in the latter
case? The parentheses are a requirement in a call to the apportion()
function, of course, but not in std::vector<> ctor call.

GC> Perhaps I was correct in one of those cases, but I don't know which.

 It hinges on definition of distaste, which is rather difficult to pin
down. Personally I definitely wouldn't use ({}). If possible, as it is with
CATCH, I'd just use {}. If not, I'd use () or a temporary variable.

 
GC> >  So I wonder about what of the following would you prefer:
GC> > 
GC> > 0. Keep using parentheses in any new code.
GC> 
GC> That would imply that 'html.?pp' as quoted above should be changed, but
GC> I don't think it should be changed.
GC> 
GC> > 1. Use braces in the new code and replace neighbouring parentheses with
GC> >    the braces in this code only.
GC> > 2. Replace all the existing parentheses with braces and use only the 
latter
GC> >    ones in the future.
GC> > 
GC> > (I omit the variant "½. Use braces in the new code without changing
GC> > anything else" as having different kind of parentheses in the same file is
GC> > just too inconsistent to be acceptable IMO).
GC> 
GC> Number two. Only number two produces global consistency.

 OK, I'll extend the Github commit shown above to cover all the files then.
Just a word of warning: it would be best to apply it relatively quickly, as
such global changes will very quickly get out of date and result in
conflicts if you try to apply it later. Would you be ready to apply such
(mostly purely mechanical) patch now? If not, please let me know and I'll
delay it.

GC> Unless we discover cases where number two doesn't work.

 It only won't work if there is a narrowing conversion. In this case we can
still perform the conversion explicitly or try to get rid of it. And either
would be an improvement compared to an implicit conversion IMHO.

 Regards,
VZ