Re: [lmi] Delegating ctor uncalled for?

[Vadim Zeitlin]

On Tue, 7 Mar 2017 13:29:00 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2017-03-03 14:18, Vadim Zeitlin wrote:
GC> > On Wed, 1 Mar 2017 12:52:58 +0000 Greg Chicares <address@hidden> wrote:
GC> > 
GC> > GC> What do you think of the patch below[0]? The question I'm struggling 
with
GC> > GC> is whether it's worth an extra fifty lines of code just to say "don't 
use
GC> > GC> class SequenceParser directly"
GC> [...]
GC> >  I'm afraid I can't say anything really original about this, but just
GC> > repeat what I had already written before: yes, I do think the patch below
GC> > improves things and I also think that [...snip...]
GC> 
GC> Focusing only on the 2017-03-01 patch...I just set out to apply it, and
GC> started by trying to write a commit message describing its purpose, but
GC> I'm stuck. What useful purpose does it accomplish?

 I agree that it doesn't seem to be really useful now.

GC> Omitting the minor details (which may worth applying in their own right),
GC> the patch changes this:
GC> 
GC>   class parser {
GC>     public:
GC>       parser(argument_type); // This is the only public ctor.
GC>       result_type results() const;
GC>   };
GC> 
GC> into this:
GC> 
GC>   class parser {
GC>     friend parser make_parser(argument_type);
GC>     public:
GC>       result_type results() const;
GC>     private:
GC>       parser(argument_type);
GC>   };
GC> 
GC>   parser make_parser(argument_type args) {return parser(args);}
GC> 
GC> In what way is that an improvement?

 What I think would be an improvement would be this, instead: just hide the
"parser" class entirely and provide only

        result_type parse(argument_type args);

function as there doesn't seem to be any benefit in having this class,
compared to a simple function, at all.

 Now the real SequenceParser class actually has 2 accessors, "intervals()"
and "diagnostics()", so it wouldn't be as simple to do the same thing with
it. In my own code I'd use something like the proposed std::expected<> (see
http://open-std.org/JTC1/SC22/WG21/docs/papers/2016/p0323r1.pdf but be
warned that this is not at all guaranteed to make it into the standard),
but I don't think you'd want to use this in lmi. And using std::pair<> for
the return value is ugly, while std::variant<> is not available in the C++
version currently used by lmi yet. So the best _realistic_ proposal I can
make is to have

        std::vector<ValueInterval> parse_sequence
                (std::string& diagnostics,
                ,std::string const& input_expression
                ,...
                );

and return an empty vector in case of failure and fill "diagnostics"
argument with the explanation.

 IMHO this would be an improvement, albeit a minor one, of course.

 Regards,
VZ