Re: [lmi] Delegating ctor uncalled for?

[Greg Chicares]

On 2017-03-07 17:46, Vadim Zeitlin wrote:
> 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.

It was difficult to guess what this class wanted to be, until many layers
of encrustation were removed.

> GC> Omitting the minor details (which may worth applying in their own right),

[applied now, BTW]

>  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.

Indeed. In the meantime, I added an inline comment suggesting how a copy
ctor and assignment operator might be implemented, based on the things
those two accessors access. (One of them accessed an ostream, so I gave
it a string to access because a string is copyable.) It could be split in
different ways--e.g., as you suggest above; or into a class that holds
only those two entities, and relegates the details to a smaller parser
class, which is the first idea that came to me--but for now I'm content
that I've marked the cleavage plane...because this has reached the point
of diminishing returns, and it's time to move on to something else.

I wrote some other ideas for future development in comments. I'm a little
sad that I didn't get to explore the idea of multiple layers of input
sequences, but a GUI for that would be a lot of work.

Looking back, I'm struck by how solid the piece that became SequenceParser
was, as opposed to the piece now called InputSequence. Evidently I spent a
lot of effort getting the parser right, whereas the other part seems to
have been a superstructure hastily erected on top of the first. Several
times I thought I had found a mistake in the parser's design and started
to "repair" it, but each time I found that it was actually right.

This exercise has also been valuable because it shows how much effort it
takes to revitalize a gnarly old component. This turned out to be a
redesign rather than a dialect modernization. Still, it's better to
redesign a working subsystem in place than to throw everything out and
try to create a whole new system from scratch.