DIP1021: Argument Ownership and Function Calls #10249

WalterBright · 2019-07-31T06:49:58Z

Initial implementation of
https://github.com/dlang/DIPs/blob/793f83911fdc8c88c6ef34e6a36b5e11e3e574e5/DIPs/DIP1021.md

dlang-bot · 2019-07-31T06:49:59Z

Thanks for your pull request, @WalterBright!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + dmd#10249"

Geod24 · 2019-07-31T08:51:09Z

src/dmd/escape.d

+            escapeByValue(arg, &eb.er);
+    }
+
+    foreach (const i, ref eb; escapeBy[0 .. $ - 1])


So this is... O((n^2)/2 * (v+r)) in the normal case (no error) (n == escapeBy.length, v == byValue.length, r == byRef.length). That doesn't sound quite right.

It's bad if there are a lot of parameters, but those cases are awfully rare.

Since there is no usage of this yet, we don't know how impactful it will be. But what we already know is that it doesn't scale at all.

ghost · 2019-08-01T23:45:33Z

Don't think this should be added until the DIP actually defines what this is suppose to do. The current DIP is less than bare bones.

wilzbach

It's okay to merge this under a preview flag. It hurts no one and gives people a real chance to play with it while evaluating the DIP. Same reasoning would also apply to other PRs for DIPs.

Geod24 · 2019-08-11T16:20:04Z

src/dmd/escape.d

+        case Tclass:
+            return true;        // even if the class fields are not mutable
+
+        case Tstruct:


This block is not covered

Geod24 · 2019-08-11T16:20:25Z

src/dmd/escape.d

+                refs = (eb.param.storageClass & (STC.out_ | STC.ref_)) != 0;
+                eb.isMutable = eb.param.isReferenceToMutable(arg.type);
+            }
+            else


This block is not covered

Geod24 · 2019-08-11T16:20:59Z

Some lines are not covered, e.g. the interaction with inout is written but not tested

codecov · 2019-08-12T13:47:29Z

Codecov Report

Merging #10249 into master will increase coverage by <.01%.
The diff coverage is 86.71%.

@@            Coverage Diff             @@
##           master   #10249      +/-   ##
==========================================
+ Coverage   81.92%   81.93%   +<.01%     
==========================================
  Files         143      143              
  Lines       74952    75080     +128     
==========================================
+ Hits        61408    61519     +111     
- Misses      13544    13561      +17

Impacted Files	Coverage Δ
src/dmd/cli.d	`0% <ø> (ø)`	⬆️
src/dmd/globals.d	`27.39% <ø> (ø)`	⬆️
src/dmd/func.d	`93.54% <ø> (ø)`	⬆️
src/dmd/declaration.d	`88.58% <100%> (+0.03%)`	⬆️
src/dmd/mars.d	`78.15% <100%> (+0.03%)`	⬆️
src/dmd/expressionsem.d	`90.4% <100%> (ø)`	⬆️
src/dmd/escape.d	`94.52% <85.95%> (-1.49%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 138b01a...2b41b23. Read the comment docs.

WalterBright · 2019-09-10T09:26:25Z

Coverage is much better now.

JinShil · 2019-09-10T09:30:50Z

src/dmd/globals.d

@@ -145,6 +145,7 @@ extern (C++) struct Param
    bool useInline = false;     // inline expand functions
    bool useDIP25;          // implement http://wiki.dlang.org/DIP25
    bool noDIP25;           // revert to pre-DIP25 behavior
+    bool useDIP1021;        // implement http://wiki.dlang.org/DIP1021


That page does not exist on the wiki.

JinShil · 2019-09-10T09:31:28Z

src/dmd/globals.h

@@ -122,6 +122,7 @@ struct Param
    bool useInline;     // inline expand functions
    bool useDIP25;      // implement http://wiki.dlang.org/DIP25
    bool noDIP25;       // revert to pre-DIP25 behavior
+    bool useDIP1021;        // implement http://wiki.dlang.org/DIP1021


That page does not exist on the wiki.

I believe you meant this URL: https://github.com/dlang/DIPs/blob/master/DIPs/DIP1021.md

Geod24 · 2019-09-10T10:28:15Z

Shall we just give up on review if you are just going to merge right after pushing? I can understand merging something that has been stalled for a while, or that has been approved and rebased without significant change, but you've been working on this for the past few hours, did 7 force pushes, and just added the automerge label without any reviews.

May I suggest using the "72 hours" label if you feel the PR has waited long enough?

WalterBright · 2019-09-10T20:50:58Z

I did fix one minor oversight in the code (delegates and function pointers were not considered). The rest of the force pushes were working on the test cases to get coverage. As for the rest, it has sat around for a month with no comments. If anyone wanted to make a comment on it, they would have by now.

JinShil · 2019-09-11T09:14:58Z

src/dmd/cli.d

@@ -713,6 +713,8 @@ dmd -cov -unittest myprog.d
            "implement https://github.com/dlang/DIPs/blob/master/DIPs/other/DIP1000.md (Scoped Pointers)"),
        Feature("dip1008", "ehnogc",
            "implement https://github.com/dlang/DIPs/blob/master/DIPs/DIP1008.md (@nogc Throwable)"),
+        Feature("dip1021", "useDIP1021",
+            "implement https://github.com/dlang/DIPs/blob/master/DIPs/other/DIP1021.md (Mutable function arguments)"),


This URL also appears to be wrong. I think you mean https://github.com/dlang/DIPs/blob/master/DIPs/DIP1021.md

Geod24 · 2019-09-13T02:46:53Z

test/fail_compilation/test1021.d

+void test2()
+{
+    int i;
+    @trusted int* toPtr(ref int i) { return &i; }


Shouldn't @safe int* toPtr(ref int i) return work here ?

Geod24

Just one minor comment (and the fact that all URL to documentation is broken). Otherwise I guess we can take this out for a spin. Did you check what effect it had on Druntime / Phobos ?

WalterBright · 2019-09-15T20:35:53Z

Fixed URLs.

WalterBright requested review from Geod24, ibuclaw, MartinNowak and rainers as code owners July 31, 2019 06:49

WalterBright force-pushed the dip1021 branch from dcd46be to 2b41b23 Compare July 31, 2019 07:27

Geod24 reviewed Jul 31, 2019

View reviewed changes

wilzbach approved these changes Aug 10, 2019

View reviewed changes

Geod24 reviewed Aug 11, 2019

View reviewed changes

src/dmd/escape.d

case Tclass:

return true; // even if the class fields are not mutable

case Tstruct:

Copy link

Member

Geod24 Aug 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block is not covered

Geod24 reviewed Aug 11, 2019

View reviewed changes

WalterBright force-pushed the dip1021 branch 7 times, most recently from ed28d71 to 87d14ca Compare September 10, 2019 09:17

WalterBright added the Merge:auto-merge label Sep 10, 2019

JinShil reviewed Sep 10, 2019

View reviewed changes

Geod24 added Merge:72h no objection -> merge The PR will be merged if there are no objections raised. and removed Merge:auto-merge labels Sep 10, 2019

JinShil reviewed Sep 11, 2019

View reviewed changes

Geod24 reviewed Sep 13, 2019

View reviewed changes

Geod24 approved these changes Sep 13, 2019

View reviewed changes

DIP1021: Argument Ownership and Function Calls

c08bf7b

WalterBright force-pushed the dip1021 branch from 87d14ca to c08bf7b Compare September 15, 2019 20:35

WalterBright added the Merge:auto-merge label Sep 15, 2019

dlang-bot merged commit 639c671 into dlang:master Sep 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DIP1021: Argument Ownership and Function Calls #10249

DIP1021: Argument Ownership and Function Calls #10249

WalterBright commented Jul 31, 2019

dlang-bot commented Jul 31, 2019

Geod24 Jul 31, 2019

WalterBright Jul 31, 2019

Geod24 Aug 2, 2019

ghost commented Aug 1, 2019

wilzbach left a comment

Geod24 Aug 11, 2019

Geod24 Aug 11, 2019

Geod24 commented Aug 11, 2019

codecov bot commented Aug 12, 2019

WalterBright commented Sep 10, 2019

JinShil Sep 10, 2019 •

edited

Loading

JinShil Sep 10, 2019

WalterBright Sep 10, 2019

JinShil Sep 11, 2019

Geod24 commented Sep 10, 2019

WalterBright commented Sep 10, 2019

JinShil Sep 11, 2019

Geod24 Sep 13, 2019

Geod24 left a comment

WalterBright commented Sep 15, 2019

DIP1021: Argument Ownership and Function Calls #10249

DIP1021: Argument Ownership and Function Calls #10249

Conversation

WalterBright commented Jul 31, 2019

dlang-bot commented Jul 31, 2019

Bugzilla references

Testing this PR locally

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Aug 1, 2019

wilzbach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Geod24 commented Aug 11, 2019

codecov bot commented Aug 12, 2019

Codecov Report

WalterBright commented Sep 10, 2019

JinShil Sep 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Geod24 commented Sep 10, 2019

WalterBright commented Sep 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Geod24 left a comment

Choose a reason for hiding this comment

WalterBright commented Sep 15, 2019

JinShil Sep 10, 2019 •

edited

Loading