This is the feedback thread for the first round of Community Review of DIP 1037, "Add Unary Operator ...".

===================================
**THIS IS NOT A DISCUSSION THREAD**

Posts in this thread must adhere to the feedback thread rules outlined in the Reviewer Guidelines (and listed at the bottom of this post).

https://github.com/dlang/DIPs/blob/master/docs/guidelines-reviewers.md

That document also provides guidelines on contributing feedback to a DIP review. Please read it before posting here. If you would like to discuss this DIP, please do so in the discussion thread:

https://forum.dlang.org/post/alsfzarhhszauufbxzqx@forum.dlang.org
==================================

You can find DIP 1037 here:

https://github.com/dlang/DIPs/blob/ba81eec84ddf0aeeb2cb652743b292455ec8c62a/DIPs/DIP1037.md

The review period will end at 11:59 PM ET on November 10, or when I make a post declaring it complete. Feedback posted to this thread after that point may be ignored.

At the end of this review round, the DIP will be moved into the Post-Community Round 1 state. Significant revisions resulting from this review round may cause the DIP manager to require another round of Community Review, otherwise the DIP will be queued for the Final Review.

==================================
Posts in this thread that do not adhere to the following rules will be deleted at the DIP author's discretion:

* All posts must be a direct reply to the DIP manager's initial post, with only two exceptions:

    - Any commenter may reply to their own posts to retract feedback contained in the original post

    - The DIP author may (and is encouraged to) reply to any feedback solely to acknowledge the feedback with agreement or disagreement (preferably with supporting reasons in the latter case)

* Feedback must be actionable, i.e., there must be some action the DIP author can choose to take in response to the feedback, such as changing details, adding new information, or even retracting the proposal.

* Feedback related to the merits of the proposal rather than to the contents of the DIP (e.g., "I'm against this DIP.") is allowed in Community Review (not Final Review), but must be backed by supporting arguments (e.g., "I'm against this DIP because..."). The supporting arguments must be reasonable. Obviously frivolous arguments waste everyone's time.

* Feedback should be clear and concise, preferably listed as bullet points (those who take the time to do an in-depth review and provide feedback in the form of answers to the questions in the documentation linked above will receive much gratitude). Information irrelevant to the DIP or which is not provided in service of clarifying the feedback is unwelcome.

Right on, this is an awesome DIP (always thought so).

I believe before you discussed operator precedence, I think since this is being billed as an operator, it should be discussed in the DIP.

Another thing, I think it should be discussed what nested expansions will do.

-Steve

On Tuesday, 27 October 2020 at 10:54:46 UTC, Mike Parker wrote:
> This is the feedback thread for the first round of Community Review of DIP 1037, "Add Unary Operator ...".

Given the map form, `expr ...`, it seems like it would be quite easy to implement the fold form, `expr BinOp ...`, using only library code. For example, the example in the DIP:

    (Tup == 10) || ...

Could also be written as

    any(only(Tup == 10 ...))

Or without Phobos:

    bool any(bool args[]...)
    {
        bool result = false;
        foreach (arg; args)
            result ||= arg;
        return result;
    }

    any(Tup == 10 ...)

The library versions require at worst O(1) template instantiations, and have the advantage that they are not limited to built-in operators. What are the advantages of `expr BinOp ...` compared to this approach?

Incidentally I'm working on a book chapter on C++ variadics and just read through the standard (https://github.com/cplusplus/draft/blob/master/papers/n4140.pdf, search for "variadic" and "pack"). It provides a good baseline for evaluating this proposal.

Overall: the proposal is imprecise and needs a lot more fleshing out in order to provide an actual specification for implementation.

Abstract: should not be an executive review consisting of only one confusing sentence ("..." is not an expression, it's punctuation or operator). Abstract should clarify how explicit tuple expansion compares with the existing implicit expansion.

* "...the mechanisms to implement them in D are awkward..." they are the standard functional approach. The DIP should at best refrain from making subjective evaluation. The high compile-time cost is good rationale.

* The proposal does not mention things like Reverse, Sort etc., which would need non-forward iteration to work efficiently and are not helped by the proposal.

* "often reaching quadratic complexity for relatively simple operations" -> a couple of (references to) examples would be great

* "...expression to perform explicit tuple expansions at the expression level, which can express..." good candidate for rephrasing

* "a unary ... syntax" -> "s a unary ... postfix operator"

* "(Tup*10)...  -->  ( Tup[0]*10, Tup[1]*10, Tup[2]*10 )" -> the example does not clarify how one expression expands into multiple expressions; this is not something that an operator does. The parens don't help - are they required, provided for illustration...? The meaning of the expansion (e.g. array initialization vs. function call etc) is determined by the context of the expansion. That's why the C++ proposal and standard focus most of the description on expansion loci.

* "C++11 implemented template parameter pack expansion with similar semantics, and it has been a great success in the language. Coupled with D's superior metaprogramming feature set, D users can gain even greater value from this novel feature." -> specious argument, even if we allow for the "great success" in C++. (Most uses of "..." in C++ are sheer black magic and have required simplifications in C++17. NOT a success story.) The main problem is different though. C++ parameter packs don't enjoy /any/ other operation aside from expansion and "...". To add that to the many existing operators for tuples that D has and claim it'll just work great because it did in C++ does not stand to reason.

* Major bug: the "Rationale" discusses only expression, whereas staticMap does not use expressions. It just processes tuples, which may contain types. Types cannot appear in expressions. C++ goes to great lengths to distinguish between template parameter packs (which may be one of type parameter pack, value parameter pack, and template template parameter pack) and function parameter packs (which may only be parameter declarations). By the Rationale nothing except expressions will be accessible to D's proposed "...". That means no staticMap for non-valies (e.g. staticMap!(Unqual, types)), which probably wasn't the intent of the DIP.

* "The implementation will explore expr" -> there's no formal definition of "explore". The C++ spec mentions "the largest expression to the left of the ...". Probably that would work here, too.

* "A second form shall exist which may implement a static reduce operation with the syntax expr [BinOp] ..." What happens if the tuple is empty? C++17 allows ... only in between operators, e.g.:

return false || ... || args == value;

thus allowing the author to choose the limit value.

* The "Compliation Performance" needs to discuss how the operator handles backward iteration.

On 10/27/20 2:33 PM, Andrei Alexandrescu wrote:
> C++ parameter packs don't enjoy /any/ other operation aside from expansion and "...".

Meant to say:

C++ parameter packs don't enjoy /any/ other operation aside from expansion and "sizeof...".

The tuple expansion is great, I love it. Its missing from the language far too long.

Personally, I find the DIP addresses the lesser of the two important use cases of expansion. Expression tuple expansion is a thing to want, but type tuple expansion is more to want.
With that DIP, it is still not possible to get from a AliasSeq!(int, long, Foo) to AliasSeq!(Array!int, Array!long, Array!Foo).

The DIP should therefore expand the proposal to type contexts.

Also, I find it should be possible to hook that operator. opExpand would be my name of choice. Basically, the DIP should include that when the `...` operator is "pushed down" the expression tree, when it hits a subexpression E such that E.opExpand compiles, it should use it as a sequence. As an example, if xs is a sequence (say, xs == AliasSeq!(1,2,3) for example) and ys is of some type that defines opExpand (alias opExpand = AliasSeq!("a","b","c"); for example), then (ys[xs])... expands into (ys.opExpand[0][xs[0]], ys.opExpand[1][xs[1]], ys.opExpand[2][xs[2]]). That way, tuples could be expanded without need to `alias this` to the underlying fields, making it possible to have them encapsulated.

I mention that it's a postfix unary operator. I think that implies the
precedence, no?
All the postfix operators work from the inside outwards. If there's
precedence levels in there that I didn't notice, then I can amend to make
it explicit.

On Tue, Oct 27, 2020 at 11:55 PM Steven Schveighoffer via Digitalmars-d < digitalmars-d@puremagic.com> wrote:

> Right on, this is an awesome DIP (always thought so).
>
> I believe before you discussed operator precedence, I think since this is being billed as an operator, it should be discussed in the DIP.
>
> Another thing, I think it should be discussed what nested expansions will do.
>
> -Steve
>

On Wed, Oct 28, 2020 at 2:20 AM Paul Backus via Digitalmars-d < digitalmars-d@puremagic.com> wrote:

> On Tuesday, 27 October 2020 at 10:54:46 UTC, Mike Parker wrote:
> > This is the feedback thread for the first round of Community Review of DIP 1037, "Add Unary Operator ...".
>
> Given the map form, `expr ...`, it seems like it would be quite easy to implement the fold form, `expr BinOp ...`, using only library code. For example, the example in the DIP:
>
>      (Tup == 10) || ...
>
> Could also be written as
>
>      any(only(Tup == 10 ...))
>
> Or without Phobos:
>
>      bool any(bool args[]...)
>      {
>          bool result = false;
>          foreach (arg; args)
>              result ||= arg;
>          return result;
>      }
>
>      any(Tup == 10 ...)
>
> The library versions require at worst O(1) template instantiations, and have the advantage that they are not limited to built-in operators. What are the advantages of `expr BinOp ...` compared to this approach?
>

The fold semantic is about 3 extra lines of code in the implementation
compared to the map form.
The main advantage is that it's terse and easy to read, less work for the
compiler (even if negligible).

Your example implementation above makes me uncomfortable; I wouldn't expect
that to inline reliably, and if it doesn't inline, then it will generate
very bad code. It won't constant-fold properly anymore, all terms will
become runtime evaluated even where they're constant, etc.
Consider `Tup && ...`, where the tuple contains a single constant `false`
element. In that (common) case, the entire expression collapses to `false`
at compile time. If, for any reason whatsoever, the compiler emits a call
to that function (inlining disabled?); the arguments will be assembled into
an array (including the constant `false`), and it will be runtime
evaluated. Same applies for `Tup || ...` with a single constant `true`
element. These are super-common cases. The entire expression reliably
collapses to a constant as per the spec, and that is not driven or
dependent on extra compiler or optimiser work at later phases.

As a user, I would be surprised if the fold expression didn't work when I
tried to type it... the rule of least surprise dictates to me that it
should exist.
Anyway, I agree what you say is possible, but I don't like it. Tuple
expansion is about static expansion; the entire point is that it's a static
expression, and your suggestion undermines the goal of this DIP as I see it.

If the DIP were going to be rejected on the basis of the fold feature, I would consider removing the fold feature. It makes no material difference to language complexity, or to the implementation; it's about 3 lines of code.

On Wed, Oct 28, 2020 at 4:35 AM Andrei Alexandrescu via Digitalmars-d < digitalmars-d@puremagic.com> wrote:

> Incidentally I'm working on a book chapter on C++ variadics and just read through the standard (https://github.com/cplusplus/draft/blob/master/papers/n4140.pdf, search for "variadic" and "pack"). It provides a good baseline for evaluating this proposal.
>
> Overall: the proposal is imprecise and needs a lot more fleshing out in order to provide an actual specification for implementation.
>
> Abstract: should not be an executive review consisting of only one confusing sentence ("..." is not an expression, it's punctuation or operator). Abstract should clarify how explicit tuple expansion compares with the existing implicit expansion.
>

I have no idea how to approach that in a spec.
I don't think I 'compare' with "existing implicit expansion", that might
even be off-topic.

In reality, my DIP doesn't perform an 'expansion' in the terms that D might
currently talk about tuple expansion. All my DIP does is to apply a
transformation to a tuple. Tuple-in, tuple-out; that's not really an
'expansion' in current language terms, it's just a map transformation on
the tuple itself.
I don't know how to fix this language.

* "...the mechanisms to implement them in D are awkward..." they are the
> standard functional approach. The DIP should at best refrain from making subjective evaluation. The high compile-time cost is good rationale.
>

I find the awkwardness to be similarly motivating as the high compile-time cost. If the CT cost is prohibitive to my program and there is no reasonable workaround, a solution like this becomes necessary, but in other cases even where there is not a high compile-time cost, I care about the awkwardness and how ugly the code is to read and write; bloaty syntax and logical indirections via shim templates often written elsewhere that I have to go and find. It's often a volume of text that overwhelms surrounding code and allows the actual point to be lost.

* The proposal does not mention things like Reverse, Sort etc., which
> would need non-forward iteration to work efficiently and are not helped by the proposal.
>

I don't understand the relevance of this point... can you show where iteration and tuples have overlapping semantics?

* "often reaching quadratic complexity for relatively simple operations"
> -> a couple of (references to) examples would be great
>

I mean, `staticMap` is the poster child, and it's the least offensive example possible to write ;)

* "...expression to perform explicit tuple expansions at the expression
> level, which can express..." good candidate for rephrasing
>

Yeah, the point made by Mr 'Q' below needs to feed into this... and I don't
know how to do it.
It's not strictly an 'expression'... although it kind-of is.

I don't know spec language to deal with these syntax trees where they are
not yet known to be expressions, or types, or... whatever.
The transformation (and tuples in general) exist as a point in compilation
where value/type concepts are not yet relevant. They become relevant when
evaluating the code that the tuple is plugged into at a later phase.
What do you call pre-determined syntax trees? What is the language to
perform operations on a yet-to-be-determined 'kind' of thing?

* "a unary ... syntax" -> "s a unary ... postfix operator"
>

Yes.

* "(Tup*10)...  -->  ( Tup[0]*10, Tup[1]*10, Tup[2]*10 )" -> the example
> does not clarify how one expression expands into multiple expressions; this is not something that an operator does. The parens don't help - are they required, provided for illustration...? The meaning of the expansion (e.g. array initialization vs. function call etc) is determined by the context of the expansion. That's why the C++ proposal and standard focus most of the description on expansion loci.
>

Actually, the parens are a bug in the DIP. It should read:
  (Tup*10)...  -->  (Tup[0]*10), (Tup[1]*10), (Tup[2]*10)

I thought that should be clear. And yes, the parens are necessary because I use a bin-op in this example which have lower precedence than unary operators.

Should I write:
   (Tup*10)...  -->  AliasSeq!((Tup[0]*10), (Tup[1]*10), (Tup[2]*10 ))

I'm not sure using `AliasSeq`, which is a piece of library, is appropriate in a spec?

Again, I try to show semantic through obvious example here because the spec language I refer to above is mysterious to me.

* "C++11 implemented template parameter pack expansion with similar
> semantics, and it has been a great success in the language. Coupled with D's superior metaprogramming feature set, D users can gain even greater value from this novel feature." -> specious argument, even if we allow for the "great success" in C++. (Most uses of "..." in C++ are sheer black magic and have required simplifications in C++17. NOT a success story.) The main problem is different though. C++ parameter packs don't enjoy /any/ other operation aside from expansion and "...".


Well... what do you want? Should I just remove that? People asked me to add it.

To add that
> to the many existing operators for tuples that D has and claim it'll just work great because it did in C++ does not stand to reason.
>

What operators do tuples have in D? We can slice and index them... I think
that's all?
Why might you imagine this DIP would interact with tuple element indexing?
Why is that significant enough to call out explicitly?

* Major bug: the "Rationale" discusses only expression, whereas
> staticMap does not use expressions. It just processes tuples, which may contain types. Types cannot appear in expressions. C++ goes to great lengths to distinguish between template parameter packs (which may be one of type parameter pack, value parameter pack, and template template parameter pack) and function parameter packs (which may only be parameter declarations). By the Rationale nothing except expressions will be accessible to D's proposed "...". That means no staticMap for non-valies (e.g. staticMap!(Unqual, types)), which probably wasn't the intent of the DIP.
>

Yes, as mentioned above, and also by Q below, this is the critical issue
with this DIP, and I have no idea how to address this.
Everywhere I use the term 'expression' is invalid, but what do I write
instead?

`syntax...` <- what is 'syntax' called in spec-language? It could be any syntax tree that makes grammatical sense.

* "The implementation will explore expr" -> there's no formal definition
> of "explore". The C++ spec mentions "the largest expression to the left of the ...". Probably that would work here, too.
>

Also an important point... how do you imagine that phrase you reference applying here? Can you suggest a better sentence?

* "A second form shall exist which may implement a static reduce
> operation with the syntax expr [BinOp] ..." What happens if the tuple is empty?


Compile error on empty tuple. I thought that was stated in the DIP, but I missed it. It states the rules on equal length, and no tuple present, but misses empty tuple >_<

C++17 allows ... only in between operators, e.g.:
>
> return false || ... || args == value;


I haven't specified this, because I don't believe it's necessary the same
way it is in C++. This is necessary in C++ because parameter packs are
barely part of the language, and painfully cumbersome to interact with. In
D, if you want a limit value, I think it would be reasonable to just append
a limit value to the tuple in-situ.
There has been a lot of discussion about first-class tuples in D, and in
that future you'd be able to do `MyTup ~ limit`, but for now
`AliasSeq!(MyTup, limit)` seems convenient enough to me that it shouldn't
require that we spec a limit syntax like C++.
If it's determined in the future that we want the limit case, it can be
trivially added with a follow-up DIP, but I would not add it eagerly, I
believe it's unnecessary.

thus allowing the author to choose the limit value.
>
> * The "Compliation Performance" needs to discuss how the operator handles backward iteration.
>

I don't understand what you're asking about here? Tuples don't 'iterate'... I'm not aware my DIP impacts or intersects any semantics dealing with iteration that should need to be called out?

...

These are all good points. Are you opposed to this DIP? If not, would you
consider collaborating on this?
I don't really know appropriate language to address some of your points.
I'm convinced the DIP will fail on account of the points you make, and if I
can't correct them, then it's an automatic fail, and I might as well
withdraw it now to save myself the torture, and you the time destroying it
in a few weeks.

Or anyone else...? This definitely needs fixing, and I don't really know how to do it.

On Tuesday, 27 October 2020 at 10:54:46 UTC, Mike Parker wrote:
> This is the feedback thread for the first round of Community Review of DIP 1037, "Add Unary Operator ...".

Avoid qualitative assumptions of whether this is a success in c++ or not. This is actually a maginal c++ feature that does not fit well with the language.

Please discuss this feature vs more powerful and generic features like comprehensions.