On Fri, 20 Jul 2018 at 03:55, Petar via Digitalmars-d <digitalmars-d@puremagic.com> wrote:
>
> On Friday, 20 July 2018 at 05:16:53 UTC, Mike Parker wrote:
> > This is the feedback thread for the first round of Community Review for DIP 1016, "ref T accepts r-values":
> >
> > https://github.com/dlang/DIPs/blob/725541d69149bc85a9492f7df07360f8e2948bd7/DIPs/DIP1016.md
> >
> > All review-related feedback on and discussion of the DIP should occur in this thread. The review period will end at 11:59 PM ET on August 3, or when I make a post declaring it complete.
> >
> > At the end of Round 1, if further review is deemed necessary, the DIP will be scheduled for another round. Otherwise, it will be queued for the Final Review and Formal Assessment by the language maintainers.
> >
> > Please familiarize yourself with the documentation for the Community Review before participating.
> >
> > https://github.com/dlang/DIPs/blob/master/PROCEDURE.md#community-review
> >
> > Thanks in advance to all who participate.
>
> First of all, I'm very glad to see Manu's persistence turn to fruition and by incorporating all the feedback so far, I think we have a very solid proposal.
>
> One question:
>
> > It is important that `T` be defined as the argument type [..]
>
> I think this should say that `T` is defined as the *parameter* type.

You're exactly right!
I'll fix it.

This is what I would expect to happen for `void fun(ref int
> x, ref long y, ref double z)`:
>
> ```
> fun(short(1), byte(2), int(3));
> ```
>
> =>
>
> ```
> {
>      int    __fun_temp0 = void;
>      long   __fun_temp1 = void;
>      double __fun_temp2 = void;
>
>      fun(
>          __fun_temp0 := short(1),
>          __fun_temp1 := byte(2),
>          __fun_temp2 := int(3)
>      );
> }
> ```

Precisely. I just used the wrong word!


> One other subtle point is that the order of declaration of the temporaries determines their lifetime, so we must be careful with that. In fact I think that we should define it so declaration and initialization are not separated and simply make the order of declaration match the function argument evaluation order.

Consider that we were calling a normal function:
  fun(T(x, y), gun(10, obj.prop), makeThing());

ref aside, the initialisation order of the temporaries should exactly match the initialisation and/or evaluation order of args to regular functions.

For instance, with an expression (x and y are objects):
    T x, y;
    fun(x + y)  -- becomes --> fun(T(x + y))
Ie, the construction should be incorporated into the usual argument
evaluation expression, and evaluated at the normal time.

I'm not sure if that evaluation order is spec-ed, or just
implementation defined?
Whichever it is, we much match that.

Can you propose text I could use to this effect? Add a PR with that sentence if you like?

On Fri, 20 Jul 2018 at 04:50, Seb via Digitalmars-d <digitalmars-d@puremagic.com> wrote:
>
> On Friday, 20 July 2018 at 05:16:53 UTC, Mike Parker wrote:
> > This is the feedback thread for the first round of Community Review for DIP 1016, "ref T accepts r-values":
> >
> > https://github.com/dlang/DIPs/blob/725541d69149bc85a9492f7df07360f8e2948bd7/DIPs/DIP1016.md
> >
> > All review-related feedback on and discussion of the DIP should occur in this thread. The review period will end at 11:59 PM ET on August 3, or when I make a post declaring it complete.
> >
> > At the end of Round 1, if further review is deemed necessary, the DIP will be scheduled for another round. Otherwise, it will be queued for the Final Review and Formal Assessment by the language maintainers.
> >
> > Please familiarize yourself with the documentation for the Community Review before participating.
> >
> > https://github.com/dlang/DIPs/blob/master/PROCEDURE.md#community-review
> >
> > Thanks in advance to all who participate.
>
> I like this too. Solid work!
>
> One minor point: the DIP could mentioned structs with disabled
> postblits (it currently only tangentially touches it by stating
> that no copy operation should happen).
> It would be great to make sure that this will work:
>
> ---
> struct A
> {
>      int i;
>      @disable this(this);
> }
>
> void fun(ref A x);
> void test()
> {
>      fun(A(42));
> }
> ---

Can I get a second opinion on this?
I think it's implicit that this works (ref args do not copy), and I
don't think an unrelated thing (copy construction) needs to be made
explicit here.

On Friday, July 20, 2018 17:25:25 Daniel N via Digitalmars-d wrote:
> On Friday, 20 July 2018 at 17:02:04 UTC, Jonathan M Davis wrote:
> > On Friday, July 20, 2018 16:42:54 aliak via Digitalmars-d wrote:
> >> On Friday, 20 July 2018 at 13:21:11 UTC, Jonathan M Davis
> >>
> >> But as for a UDA, maybe @implicit from the copy constructor DIP can be reused here?
> >>
> >> void f(@implicit ref A a) {}
> >
> > I don't know. Maybe. I'd certainly prefer @rvalue, since it would be clearer (and slightly shorter for that matter), and I don't really agree with copy constructors requiring @implicit anyway. But at the moment, I don't see a technical reason why the attribute couldn't be reused.
>
> I find the DIP addresses this in a cleaner way with opt-out.
> 1) Avoids adding more UDA:s
> 2) It's probably more common to wish to use this feature than the
> opposite, i.e. by taking the opt-out route we can significantly
> reduce @uda clutter.
>
> See DIP:
> > void lval_only(int x) @disable;
> > void lval_only(ref int x);
> >
> > int x = 10;
> > lval_only(x);  // ok: choose by-ref
> > lval_only(10); // error: literal matches by-val, which is
> > @disabled

It changes the semantics of existing code, and honestly, it seems ridiculous to me to have to declare a separate function to make ref work the way that it's always worked.

And while I'm sure that if this is implemented, some folks will rush out and start slapping ref all over the place (or @rvalue ref if it were actually done with a new attribute like it should be IMHO), the reality of the matter is that whether creating a temporary to pass rvalues by ref or simply having the function accept everything by value is better is not straightforward. In many cases, passing by value is more efficient - especially when lots of rvalues are involved and relatively few lvalues. Even C++ has changed its tune on this as far as best practices go. With C++98, it was considered best practice to use const& all over the place, whereas once they added move constructors, the situation became much more complicated, and as I understand it, it's now often considered best practice to pass by value unless you find that you need to do otherwise, because using const& tends to prevent moves and often results in extra copies being made. So, while being able to pass rvalues by ref may be useful in many cases, using @rvalue ref all over the place is unlikely to be a good idea in general. So, I don't think that it's at all safe to say that allowing ref accept rvalues by ref is going to reduce the amount of UDA clutter in the average D program - especially when @rvalue would mean just adding an attribute, whereas what the DIP proposes involves having to declare an extra overload just to disable it.

Either way, what this DIP proposes means that existing uses of ref will need to be changed - and in an extremely verbose way - in order to get back their current behavior.

- Jonathan M Davis

On Fri, 20 Jul 2018 at 05:40, Steven Schveighoffer via Digitalmars-d <digitalmars-d@puremagic.com> wrote:
>
> On 7/20/18 1:16 AM, Mike Parker wrote:
> > This is the feedback thread for the first round of Community Review for DIP 1016, "ref T accepts r-values":
> >
> > https://github.com/dlang/DIPs/blob/725541d69149bc85a9492f7df07360f8e2948bd7/DIPs/DIP1016.md
> >
> >
> > All review-related feedback on and discussion of the DIP should occur in this thread. The review period will end at 11:59 PM ET on August 3, or when I make a post declaring it complete.
> >
> > At the end of Round 1, if further review is deemed necessary, the DIP will be scheduled for another round. Otherwise, it will be queued for the Final Review and Formal Assessment by the language maintainers.
> >
> > Please familiarize yourself with the documentation for the Community Review before participating.
> >
> > https://github.com/dlang/DIPs/blob/master/PROCEDURE.md#community-review
> >
> > Thanks in advance to all who participate.
>
> Looks pretty good to me.
>
> There is very little discussion of the original problem (the reason why ref does not bind to rvalues). I don't know if this will satisfy Walter.

I'm not aware of any substantial discussion?
I understand the design is to prevent accidental disposal of function results.

> A couple points to make there:
>
> 1. `this` has been binding as ref to rvalues since structs received `this` by ref (yes, they were pointers at one point), and there really have not been averse effects.

Interesting. I'm not sure how to write this in a sentence, can you produce a patch or PR?

> 2. The case is made to allow return ref pipeline functionality, but does not address the other cases. A strawman to discuss is a function which takes a value-type by reference, and returns void. It's possible the function squirrels away a copy of the value somewhere, but unlikely (why would it take it by ref in that case?). I'd say 99% of the time the point is to modify the parameter for use after the function ends. But in the rvalue case, you will lose it immediately. So in effect a call that seemingly has some effect will have none.

Do you really think words on this are valuable? It's already a very long DIP.
There is comment that the original design was intended to prevent
accidental loss of function results, and the rest of the DIP tries to
establish that that decision is out of date.
I think the discussion you are asking for is there... perhaps you can
suggest how you'd be satisfied here?

> I would point out that 1 here serves as an example of why 2 isn't critical -- you can easily get into this exact situation WITHOUT the DIP if the function in question is a member function. And people have stumbled across such issues, fixed the problems, and moved on. I would say from personal experience the occurrence of such bugs is pretty rare.
>
> I would also point out that the issues with generic code that have been pointed out already apply here as well -- generic code might NOT CARE if the function has any effect, but having to do metacrobatics to call it the "right way" or avoid calling it on purpose would be painful to write (as has been demonstrated).

I understand your points. I'm not sure I have a sense of the text
you'd like to read though.
If you'd like to suggest a specific patch, or PR, that'd be get me on
the same wavelength :)

> One further point, on the default arguments: you can have a default argument be ref, so make sure that section does not read like it *always* has to create a temporary.

Got it.

On Fri, 20 Jul 2018 at 06:21, Jonathan M Davis via Digitalmars-d <digitalmars-d@puremagic.com> wrote:
>
> On Friday, July 20, 2018 05:16:53 Mike Parker via Digitalmars-d wrote:
> > This is the feedback thread for the first round of Community Review for DIP 1016, "ref T accepts r-values":
> >
> > https://github.com/dlang/DIPs/blob/725541d69149bc85a9492f7df07360f8e2948bd 7/DIPs/DIP1016.md
> >
> > All review-related feedback on and discussion of the DIP should occur in this thread. The review period will end at 11:59 PM ET on August 3, or when I make a post declaring it complete.
> >
> > At the end of Round 1, if further review is deemed necessary, the DIP will be scheduled for another round. Otherwise, it will be queued for the Final Review and Formal Assessment by the language maintainers.
> >
> > Please familiarize yourself with the documentation for the Community Review before participating.
> >
> > https://github.com/dlang/DIPs/blob/master/PROCEDURE.md#community-review
> >
> > Thanks in advance to all who participate.
>
> I am completely against allowing ref to accept rvalues without some sort of attribute indicating that it should be allowed to (e.g. @rvalue ref).

I sincerely hope you're in the minority :(

> Allowing ref to accept rvalues goes completely against the idea that ref is for passing an object so that it can be mutated and have its result affect the caller.

That is *one use* for ref. I produced text to the effect of changing your mental model and such assertions.

"This DIP proposes that we reconsider the choice to receive an argument by value or by reference as a detail that the API *author* selects with respect to criteria relevant to their project or domain."

Could I improve communication of that sentiment?

> With this DIP, we'd likely start seeing folks using ref all over
> the place even when it has nothing to do with having the function mutating
> its arguments,

Absolutely. 'ref' is arbitrarily suppressed in today's language.

> and that's not only error-prone, but it obfuscates what ref was originally intended for.

Ref's original 'intent' was narrower than it's practical use. We're specifically trying to relax the restrictions to respect that reality.

> auto ref was already introduced to solve this problem. The problem of course is that it requires that the function be templated, and while that's often desirable, it's not always a viable option (e.g. it doesn't work with virtual functions). So, I'm fine with adding something akin to auto ref which is intended to solve the non-templated case with semantics similar to those described in the DIP, but I think that it would be a huge mistake to make normal ref accept rvalues.

If the function uses ref to return results, many such cases use can 'out'.
Functions that mutate the argument to return results aren't
automatically a dismissal of results; perhaps the function is impure,
or it receives multiple args (and one is uninteresting).
It is legitimate to discard the outputs of any function, whether you
ignore the functions return value OR whether you ignore results output
via ref.
Ignoring the return value raises the same argument, but you don't have
this reaction in that case.

It's too strict to frustrate all other useful applications of ref because of a strict application of this arbitrary rule.

> IMHO, having it be an error to pass an rvalue by ref is often a very valuable behavior, and I do _not_ want to lose that. I would be fine if this DIP proposed an attribute such as @rvalue to enable ref to accept rvalues, but I very much hope that this DIP is not accepted so long as it allows ref to accept rvalues without such an attribute.

This DIP proposes how to retain your desired passing-rvalue-is-an-error feature, and also demonstrates the symmetrical mechanism for lvalues too.

On Fri, 20 Jul 2018 at 11:05, Jonathan M Davis via Digitalmars-d <digitalmars-d@puremagic.com> wrote:
>
> On Friday, July 20, 2018 17:25:25 Daniel N via Digitalmars-d wrote:
> > On Friday, 20 July 2018 at 17:02:04 UTC, Jonathan M Davis wrote:
> > > On Friday, July 20, 2018 16:42:54 aliak via Digitalmars-d wrote:
> > >> On Friday, 20 July 2018 at 13:21:11 UTC, Jonathan M Davis
> > >>
> > >> But as for a UDA, maybe @implicit from the copy constructor DIP can be reused here?
> > >>
> > >> void f(@implicit ref A a) {}
> > >
> > > I don't know. Maybe. I'd certainly prefer @rvalue, since it would be clearer (and slightly shorter for that matter), and I don't really agree with copy constructors requiring @implicit anyway. But at the moment, I don't see a technical reason why the attribute couldn't be reused.
> >
> > I find the DIP addresses this in a cleaner way with opt-out.
> > 1) Avoids adding more UDA:s
> > 2) It's probably more common to wish to use this feature than the
> > opposite, i.e. by taking the opt-out route we can significantly
> > reduce @uda clutter.
> >
> > See DIP:
> > > void lval_only(int x) @disable;
> > > void lval_only(ref int x);
> > >
> > > int x = 10;
> > > lval_only(x);  // ok: choose by-ref
> > > lval_only(10); // error: literal matches by-val, which is
> > > @disabled
>
> It changes the semantics of existing code,

No *existing* code 'accidentally' dismissed results of ref args. It doesn't actually change semantics of existing code. It changes semantics of code you could potentially write in the future.

> and honestly, it seems ridiculous
> to me to have to declare a separate function to make ref work the way that
> it's always worked.

This is exactly what @disable is designed and good for.
This feels like an excellent and intuitive choice to me. I think this
is an improvement by itself, irrespective of this DIP.

> And while I'm sure that if this is implemented, some folks will rush out and start slapping ref all over the place (or @rvalue ref if it were actually done with a new attribute like it should be IMHO), the reality of the matter is that whether creating a temporary to pass rvalues by ref or simply having the function accept everything by value is better is not straightforward. In many cases, passing by value is more efficient - especially when lots of rvalues are involved and relatively few lvalues. Even C++ has changed its tune on this as far as best practices go. With C++98, it was considered best practice to use const& all over the place, whereas once they added move constructors, the situation became much more complicated, and as I understand it, it's now often considered best practice to pass by value unless you find that you need to do otherwise, because using const& tends to prevent moves and often results in extra copies being made.

This DIP isn't a discussion on efficiency. 'best practise' is a
conversation that will continue to be ongoing (and is highly context
sensitive). Fortunately, D has a more mature pre-disposition relative
to C++ on this matter, so I expect the explosion of ref you predict
will not be so explosive as you imagine making direct comparison to
C++.
The point this DIP makes is that, choice to use 'ref' is a decision
_for the API author_, and for reasons that context sensitive.

Whether ref should or shouldn't be used is not your prescription to
make, and while there may be discussions about efficiency, I think
there are many more cases where the choice has nothing to do with
efficiency.
In many cases, it's already prescribed by 3rd party, and you have no choice.

> So, while being
> able to pass rvalues by ref may be useful in many cases, using @rvalue ref
> all over the place is unlikely to be a good idea in general. So, I don't
> think that it's at all safe to say that allowing ref accept rvalues by ref
> is going to reduce the amount of UDA clutter in the average D program -
> especially when @rvalue would mean just adding an attribute, whereas what
> the DIP proposes involves having to declare an extra overload just to
> disable it.

I think disabling it is going to be the overwhelmingly niche case, but it's good that you can still do it and be satisfied if that's what you want to do.

Can you link me to a single line of your own code where you've used this pattern?

> Either way, what this DIP proposes means that existing uses of ref will need to be changed - and in an extremely verbose way - in order to get back their current behavior.

Their currently behaviour is mostly wrong, and the exact thing I'm trying to fix though.

On Friday, July 20, 2018 11:50:41 Manu via Digitalmars-d wrote:
> > I am completely against allowing ref to accept rvalues without some sort of attribute indicating that it should be allowed to (e.g. @rvalue ref).
> I sincerely hope you're in the minority :(
>
> > Allowing ref to accept rvalues goes completely against the idea that ref is for passing an object so that it can be mutated and have its result affect the caller.
>
> That is *one use* for ref. I produced text to the effect of changing your mental model and such assertions.

It's the primary use for ref as it stands given that ref does not currently accept rvalues, and personally, when I use ref on a function, it's a very specific API decision where it really does not make sense to accept lvalues. It's also how plenty of other folks use ref (e.g. it's generally how Phobo uses ref). This DIP doesn't change any of that. It just makes it harder in favor of providing a way to pass rvalues by ref. IMHO, those two use cases are distinct and should be distinct in the API. Using @disable as the DIP suggests is ridiculously verbose in comparison to how ref currently works, and it would require updating many existing uses of ref just to avoid the bugs caused by accepting rvalues. IMHO, it makes far more sense to require an explicit attribute to indicate that a parameter accepts rvalues. It's less error-prone, less verbose, and doesn't cause problems for existing code.

> "This DIP proposes that we reconsider the choice to receive an argument by value or by reference as a detail that the API *author* selects with respect to criteria relevant to their project or domain."
>
> Could I improve communication of that sentiment?

The reality of the matter is that it's always going to be up to the API author on some level - e.g. even if the proposed changes were implemented, there's still the question of whether a function's parameter should be marked with ref or not, and arguably, in general, it really shouldn't be, because it destroys the compiler's ability to move. Yes, by either allowing ref to accept rvalues or by adding an attribute like @rvalue to make ref accept rvalues, it's then up to the caller as to whether they pass an lvalue or rvalue, but it's still up to the API designer to decide how values are passed, and in some cases, it really does matter whether rvalues are accepted or not.

- Jonathan M Davis

On Friday, July 20, 2018 12:21:42 Manu via Digitalmars-d wrote:
> I think disabling it is going to be the overwhelmingly niche case, but it's good that you can still do it and be satisfied if that's what you want to do.
>
> Can you link me to a single line of your own code where you've used this pattern?

What, having ref mutate its argument and that being its entire purpose for being there? How about almost every time I have ever used ref ever. e.g. this is exactly what some functions in dxml do, and it would be just plain wrong to pass an rvalue - e.g. the overload of parseDOM that operates on an EntityRange:

http://jmdavisprog.com/docs/dxml/0.3.2/dxml_dom.html#.parseDOM

or a function like writeTaggedText

http://jmdavisprog.com/docs/dxml/0.3.2/dxml_writer.html#.writeTaggedText

which writes to the XMLWriter that it's given. Having either of those accept rvalues would just cause bugs.

In the few cases where it seems appropriate to accept both rvalues and lvalues without copying either, I can almost always use auto ref, because I rarely use classes. I agree that being able to accept lvalues without copying them would be useful for non-templated functions as well, but I do not at all agree that that is how ref should work in general. IMHO, as with auto ref, it really should be marked with an attribute to indicate that it is purposefully accepting rvalues. I would not consider it a niche case at all for ref to not accept rvalues.

> > Either way, what this DIP proposes means that existing uses of ref will need to be changed - and in an extremely verbose way - in order to get back their current behavior.
>
> Their currently behaviour is mostly wrong, and the exact thing I'm trying to fix though.

Why would the current behviour be mostly wrong? If the purpose of using ref is to accept the current value of a variable and mutate it such that the argument is mutated, then the current behaviour is _exactly_ what's desirable, and it prevents bugs.

- Jonathan M Davis

On Fri, 20 Jul 2018 at 12:36, Jonathan M Davis via Digitalmars-d <digitalmars-d@puremagic.com> wrote:
>
> On Friday, July 20, 2018 11:50:41 Manu via Digitalmars-d wrote:
> > > I am completely against allowing ref to accept rvalues without some sort of attribute indicating that it should be allowed to (e.g. @rvalue ref).
> > I sincerely hope you're in the minority :(
> >
> > > Allowing ref to accept rvalues goes completely against the idea that ref is for passing an object so that it can be mutated and have its result affect the caller.
> >
> > That is *one use* for ref. I produced text to the effect of changing your mental model and such assertions.
>
> It's the primary use for ref as it stands given that ref does not currently accept rvalues,

I'm not sure this is true.
I encounter ref constantly. It's just awkward and uncomfortable to use
rvalues => it's awkward and uncomfortable to use ref.
And I find it's comparatively rarely used for the thing you say.

I think 'primary' use is not what you say, and it's just that we all
suffer under the current rules.
Those that don't understand my pain probably just don't interact with
ref much at all. You can't have a strong opinion on this matter if you
rarely interact with ref.

I occasionally use it for the thing you say, but that's definitely a
minority case, and even in that case, I this rule has no affect on the
problem.
I have *never ever, in my life* 'accidentally' discarded an output
that I needed to use by passing an rvalue to an output-ing ref.

Think about it, that doesn't even structurally make sense.
If the function returns results via a ref, then the only way to refer
to the results it to refer to the lvalue that I passed in.
If I passed an rvalue, then it would be impossible to refer to the
results, and I would be unable to write the next line of code.

The compiler doesn't need to 'help' me here, because I literally can't
progress if I accidentally passed an rvalue.
If I successfully did pass an rvalue and dismiss the results, then
it's implied by the fact my code compiles and runs that I didn't need
the result; in that case, is no different than ignoring a return value
(which is perfectly valid and acceptable practise).

I'm sure it's *possible* to construct a case where you exhibit an undesirable effect, but we're getting into such unlikely and obviously contrived territory, that I don't see how any reasonable person could find that the hindrance caused in all other cases by the current design is in balance.

> and personally, when I use ref on a function, it's a very
> specific API decision where it really does not make sense to accept lvalues.
> It's also how plenty of other folks use ref (e.g. it's generally how Phobo
> uses ref). This DIP doesn't change any of that. It just makes it harder in
> favor of providing a way to pass rvalues by ref.

Sorry... what is this DIP making harder?

> IMHO, those two use cases
> are distinct and should be distinct in the API. Using @disable as the DIP
> suggests is ridiculously verbose in comparison to how ref currently works,
> and it would require updating many existing uses of ref just to avoid the
> bugs caused by accepting rvalues.

I think the 'bug' is implicitly resolved by the fact that you can't
make use of the results if you pass an rvalue anyway.
Like, there's no conceivable situation where a user thought they
authored their code correctly, but passing an rvalue by accident
allowed their code to compile.

Consider:
  void getOutput(ref T x);
  void doStuff(T x);

  T output;
  getOutput(output); // returns output
  doStuff(output); // consumes output

This is the 'bug' scenario:
  getOutput(makeT()); // returns output to rvalue
  doStuff( ...? ); // what do you even type here? the 'bug' resolves
itself implicitly by the programmer having to type something in this
gap

> IMHO, it makes far more sense to require
> an explicit attribute to indicate that a parameter accepts rvalues. It's
> less error-prone, less verbose, and doesn't cause problems for existing
> code.

Adding yet-another-attribute is another form of swapping one kind of
metacrobatics (to lift Schveighoffer's term) with a different kind,
and then this DIP is self-defeating.
If you want to synth a forwarding function, or something in that
domain, you now need to detect and mirror another `@rvalue` attribute
(guaranteed text mixin)... maybe this leads to `auto @rvalue`?

It's chaos.

On Friday, 20 July 2018 at 05:16:53 UTC, Mike Parker wrote:
> This is the feedback thread for the first round of Community Review for DIP 1016, "ref T accepts r-values":
>
> https://github.com/dlang/DIPs/blob/725541d69149bc85a9492f7df07360f8e2948bd7/DIPs/DIP1016.md

Yay! Thank you Manu for taking the time to write the DIP, and I greatly hope that it will get accepted and implemented quickly.

Code interacting with C++ libraries will become infinitely more readable, and from the endless past discussions on the topic no real drawback ever emerged, in my opinion.