July 16, 2019
I had written up a comment all about the DIP.
And then I reread it.

This line seems to be very important and I missed it the first time: "The checks would only be enforced for @safe code.".

If the semantics are indeed only valid in @safe code, the single example in the DIP does not show this to be the case.

It would be nice to have some pseudo-code based rules like the Rust one.
Just to make it clear in addition to some worked examples as to what is going on valid/invalid wise and how it ties into DIP25 and DIP1000.

July 15, 2019
On 7/15/2019 8:04 PM, Brad Roberts wrote:
> On 7/15/2019 6:55 PM, Walter Bright via Digitalmars-d wrote:
>> But *please do not discuss the blog post or its topic in this thread*! Use the post in the Announce forum for that:
>>
>> https://forum.dlang.org/post/hetwxvibgqcmoltvnoda@forum.dlang.org
> 
> Wait, what?  Why are we encouraging / having _discussions_ on the _announce_ list?  I know we've never prevented them in the past, but this is probably the first time I've seen active encouragement.  While I'm not an particularly active member of the community these days, I strongly disagree with this policy.

It was mainly to avoid people having to read both threads.
July 16, 2019
On Tuesday, 16 July 2019 at 06:14:02 UTC, Walter Bright wrote:
> On 7/15/2019 8:04 PM, Brad Roberts wrote:
>> On 7/15/2019 6:55 PM, Walter Bright via Digitalmars-d wrote:
>>> But *please do not discuss the blog post or its topic in this thread*! Use the post in the Announce forum for that:
>>>
>>> https://forum.dlang.org/post/hetwxvibgqcmoltvnoda@forum.dlang.org
>> 
>> Wait, what?  Why are we encouraging / having _discussions_ on the _announce_ list?  I know we've never prevented them in the past, but this is probably the first time I've seen active encouragement.  While I'm not an particularly active member of the community these days, I strongly disagree with this policy.
>
> It was mainly to avoid people having to read both threads.

To everyone discussing this aspect of my post: It's off topic! Since I obviously was not clear in my intent, I'll refrain from cutting these posts and pasting them in a new thread.

To clarify, discussion of Walter's blog post is off-topic in this thread. If you want to provide feedback on the blog post, the place to do so is in the thread where I announced it (as is often the case) or in a new thread in General. Just not here.

If you want to discuss how DIP 1021 relates to the topic of the blog post to clarify your understanding of the DIP, this review thread is fine.

If you want to discuss if we should discuss things in the announce forum, please start a new thread!

I only ask this because it can be time-consuming to read through long review threads trying to pick out the relevant information for my review summary. And for anyone coming to the review thread later, they shouldn't have to wade through irrelevant discussion to get to the good stuff.

Thanks!

So. Back on topic.
July 16, 2019
You might want to add to the rationale section that this DIP should help the compiler to produce better optimizations, as it does not need to check for pointer aliasing.

July 16, 2019
Will this be able to detect delegates which close over variables? 
 Consider this snippet (which, incidentally, also serves as a POC for @safe since it compiles successfully under @safe):

byte bb; //must be global so its address can be taken
void foo(ref byte *b, void delegate() @safe fun) {
        fun();
        *b = 4;
}
void test() {
        byte *s = &bb;

        foo(s, {s = null;});
}


It seems like a bit of an intractable problem to detect.
July 16, 2019
On 7/16/2019 2:25 PM, Elronnd wrote:
> It seems like a bit of an intractable problem to detect.

Not really. It's doable.
July 17, 2019
On Monday, 15 July 2019 at 15:23:32 UTC, Mike Parker wrote:
> This is the feedback thread for the first round of Community Review for DIP 1021, "Argument Ownership and Function Calls":
>
> https://github.com/dlang/DIPs/blob/793f83911fdc8c88c6ef34e6a36b5e11e3e574e5/DIPs/DIP1021.md

+1

I don't mind the braking change. Functions taking multiple mutable references are always suspect, so marking them @trusted or @system is a good idea anyway.

July 17, 2019
What is even being reviewed here? The DIP isn't ready for review obviously. It doesn't specify or define how anything is going to work, there is nothing to review!

It prompts replies like this:

On Wednesday, 17 July 2019 at 01:49:49 UTC, Walter Bright wrote:
> On 7/16/2019 2:25 PM, Elronnd wrote:
>> It seems like a bit of an intractable problem to detect.
>
> Not really. It's doable.

Okay, how is it doable, explain how it will be doable. I feel like any explanation that is given by the author won't be thoroughly thought out and will contain obvious holes in the explanation. The blog post mentions @live, there's nothing about that in the DIP. Not sure if you can even call this a "review". I feel this is just another DIP that will be rushed through to the finish line where it will be accepted irregardless of the DIP.



Anyways. There's no examples. No possible workarounds. Missing details for basically everything.

Per the description of the DIP:

> The checks would only be enforced for @safe code.

Yet the rationale only gives an example using C's free(). Which can't be used in @safe.

July 17, 2019
On Wednesday, 17 July 2019 at 12:30:34 UTC, Exil wrote:
> What is even being reviewed here? The DIP isn't ready for review obviously. It doesn't specify or define how anything is going to work, there is nothing to review!

I agree with your points, but I think you could have worded them better. As it is, you come across as a little rude and abrasive.

Reviews should be ruthless, but diplomatic.
July 17, 2019
On Monday, 15 July 2019 at 15:23:32 UTC, Mike Parker wrote:
> This is the feedback thread for the first round of Community Review for DIP 1021, "Argument Ownership and Function Calls":
>
> https://github.com/dlang/DIPs/blob/793f83911fdc8c88c6ef34e6a36b5e11e3e574e5/DIPs/DIP1021.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 July 30, or when I make a post declaring it complete.

I want to strongly state that this DIP is too vague and unpolished to justify its integration to the language standard, provided it's being judged by the same standards as previous DIPs.

In fact, this DIP reminds me a lot of DIP 1016 (the "rvalue ref" one), in that it suffers from similar problems.

To quote my post-rejection analysis[1] of DIP 1016:

> Now, you might say (and have said in the past) "Well that's obvious, you just need to do X, unless Y, in which case Z", but the point is that all these considerations *need to be in the proposal*.
>
> As it is, the proposal doesn't have any negative space. That is, it explains what the feature should look like, but it doesn't define boundaries, where the feature is or isn't allowed to be used.
>
> Defining negative space is important because it's how you find corner cases, cases where the correct behavior seems obvious, and yet when you spell it out it turns out everyone disagrees on how it should be handled.
>
> Right now, the DIP doesn't address potential corner cases [at all].

In particular, one area in which this proposal is lacking is examples.

Now, Walter has expressed that a spec document isn't made any clearer by a large number of examples, or by having more complex examples, and I mostly agree.

But the problem with the current examples is that they don't cover any changes in behavior introduced by the proposal, let alone corner cases. There should be at least one example showing @safe code that compiles before DIP 1021, and gives an error message after the proposal. The example would show a simplistic RAII-style container, with @safe or @trusted getters and setters; something like:

    struct S1 {
        byte* ptr;

    @safe:
        ~this() { reset(); }
        ref byte get() { return *ptr; }

    @trusted:
        void init() { ptr = cast(byte*)malloc(1); *ptr = 0; }
        void reset() { free(ptr); ptr = null; }
    }

The current example isn't good enough. It's not @safe (it calls free() directly), it's not RAII (the structure doesn't manage its own memory), and it doesn't even show what a syntax error would look like.

Ideally, there should also be additional examples illustrating nuances of the proposal; eg, since the proposal is meant to apply to ref and not pointers, there should be examples showing code that compiles with pointers and doesn't compile with refs.

Examples aside, this proposal is also way too vague and informal. The proposal says inter-statement checking isn't done, but it doesn't explain what is or isn't inter-statement checking. Do nested expressions count? Are you allowed to do this:

    foo(identity(&s), s.get()); // Does this compile ?

To quote Andrei's post-rejection analysis[2] of DIP 1016:

> It has multiple problems of informality and vague language that have worked together to cause said misunderstanding. [...]
>
> The DIP must convince the reader, and in a way the reader does not "owe" the DIP. [...] Of course we don't want to be as harsh as academics could get, but we don't want to transform DIP acceptance into a farmers market bargaining process.
>
> [...] In a thorough submission, however, there would have been many places that clear that up:
>
> * Use of a distinct notation (non-code non-text font for metalanguage, i.e. general expressions);
>
> * Description of the typechecking process, with examples of code that passes and code that fails;
>
> * Several places in text in which it is explained that rvalues resulted from implicit conversions are not eligible;

I think a lot of advice in Andrei's review is good advice for writing any DIP, and I think it absolutely applies to this one.

[1]: https://forum.dlang.org/post/hxqjetbtzlzzaxnpwgyk@forum.dlang.org
[2]: https://forum.dlang.org/post/q2u429$1cmg$1@digitalmars.com
1 2 3 4 5 6 7 8