Allow ref-returns of shared Variables #10412

UplinkCoder · 2019-09-16T18:37:36Z

This is okay because we are passing a pointer and not the acutal value.

dlang-bot · 2019-09-16T18:37:38Z

Thanks for your pull request, @UplinkCoder!

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✗	20195	normal	-preview=nosharedaccess has some access problems

Testing this PR locally

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

dub run digger -- build "master + dmd#10412"

TurkeyMan · 2019-09-16T18:59:52Z

src/dmd/statementsem.d

@@ -3161,7 +3161,10 @@ else
                rs.exp = inferType(rs.exp, fld.treq.nextOf().nextOf());

            rs.exp = rs.exp.expressionSemantic(sc);
-            rs.exp.checkSharedAccess(sc);
+            if(inferRef || !tf.isref)


Why if inferRef? What case does that represent?

inferRef means auto ref.
In which case we don't want to allow unsolicited shared access.

Why should auto ref not infer ref?

Because then code you have no control over affects the behavior of shared

How so, it should just do the appropriate shared checks after inferring ref/value. Why should there any differences between foo and bar?

void main() { shared bool b; foo(b); bar(b); } void foo(ref shared bool b) { ... } void bar()(auto ref shared bool b) { .... }

Because the ref was explicitly written and will always be ref.

That will be the case for bar as well because reading actual shared data would't compile at alll.

shared is all about making you AWARE of problem with sharing data

When using auto ref you already have to make the defensive assumption that it infers ref, not the other way round. Raising awareness is certainly a good intention but it doesn't justify odd special cases IMHO.

Anyway, this can also be deferred into another PR as the current change provides a net benefit already.

I agree that the exception is weird. If auto ref infers ref, then the call should work normally.

12345swordy · 2019-09-16T21:13:17Z

Does this require a spec change?
cc @thewilsonator

TurkeyMan · 2019-09-16T22:29:00Z

-preview=restrictiveshared is not spec-ed, it's experimental.

thewilsonator · 2019-09-17T00:22:23Z

Does this require a spec change?

No, it requires a test though.

TurkeyMan · 2019-09-17T01:48:40Z

No, it requires a test though.

Definitely.

thewilsonator · 2019-10-19T11:27:30Z

@UplinkCoder ping

thewilsonator · 2020-06-01T01:58:58Z

@UplinkCoder ping

UplinkCoder · 2020-06-06T10:43:39Z

@thewilsonator Ah yeah sorry.
Will add tests.

UplinkCoder · 2020-06-06T11:30:12Z

Because then code you have no control over affects the behavior of shared

…

On Sat, Jun 6, 2020, 12:50 PM Florian ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/dmd/statementsem.d <#10412 (comment)>: > @@ -3161,7 +3161,10 @@ else rs.exp = inferType(rs.exp, fld.treq.nextOf().nextOf()); rs.exp = rs.exp.expressionSemantic(sc); - rs.exp.checkSharedAccess(sc); + if(inferRef || !tf.isref) Why should auto ref not infer ref? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#10412 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAVWSCEY4VXTGQLSX6AMJ4DRVINPJANCNFSM4IXFZ7XA> .

UplinkCoder · 2020-06-06T12:01:53Z

Because the ref was explicitly written and will always be ref. shared is all about making you AWARE of problem with sharing data. Am Sa., 6. Juni 2020 um 13:56 Uhr schrieb Florian <notifications@github.com

…

: ***@***.**** commented on this pull request. ------------------------------ In src/dmd/statementsem.d <#10412 (comment)>: > @@ -3161,7 +3161,10 @@ else rs.exp = inferType(rs.exp, fld.treq.nextOf().nextOf()); rs.exp = rs.exp.expressionSemantic(sc); - rs.exp.checkSharedAccess(sc); + if(inferRef || !tf.isref) Because then code you have no control over affects the behavior of shared How so, it should just do the appropriate shared checks after inferring ref/value. Why should there any differences between foo and bar? void main() { shared bool b = void; foo(b); bar(b); } void foo(ref shared bool b) { atomicStore(b, false); } void bar()(auto ref shared bool b) { atomicStore(b, false); } — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#10412 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAVWSCEU4LHCKUJBSXULPVLRVIVHJANCNFSM4IXFZ7XA> .

UplinkCoder · 2020-06-06T14:28:55Z

@thewilsonator Done.

thewilsonator · 2020-06-06T14:41:10Z

@UplinkCoder I'll leave you to readd auto-merge if you need to force push more.

UplinkCoder · 2020-06-06T14:42:42Z

@thewilsonator can't do.
I am not a member of the dmd group

WalterBright · 2020-06-06T21:15:05Z

This looks like https://issues.dlang.org/show_bug.cgi?id=20195

If it is, please adjust the title and commit message accordingly.

WalterBright

This looks like https://issues.dlang.org/show_bug.cgi?id=20195

If it is, please adjust the title and commit message accordingly.

… fix) This is okay because we are passing a pointer and not the actual value. Currently excludes `ref` inferred from `auto ref`.

MoonlightSentinel · 2020-07-02T11:55:24Z

Updated commit title to include the bug reference and the commit message to state the current limitation regarding auto ref.

Commit was updated

Geod24 · 2020-07-02T15:18:23Z

Auto-ref hasn't been addressed tho

Geod24 · 2020-09-10T07:10:27Z

Did this fix the issue ? Because it's still open.

TurkeyMan reviewed Sep 16, 2019

View reviewed changes

UplinkCoder force-pushed the ref_shared branch from 76ae99e to ba4c25b Compare September 16, 2019 19:07

MoonlightSentinel added the Review:Needs Tests label Apr 8, 2020

UplinkCoder force-pushed the ref_shared branch from ba4c25b to 5278915 Compare June 6, 2020 14:28

thewilsonator approved these changes Jun 6, 2020

View reviewed changes

thewilsonator added Merge:auto-merge and removed Review:Needs Tests labels Jun 6, 2020

UplinkCoder force-pushed the ref_shared branch from 5278915 to 59a1aa0 Compare June 6, 2020 14:39

dlang-bot removed the Merge:auto-merge label Jun 6, 2020

thewilsonator added the Merge:auto-merge label Jun 6, 2020

WalterBright previously requested changes Jun 6, 2020

View reviewed changes

PetarKirov removed the Merge:auto-merge label Jun 7, 2020

Issue 20195 - Allow explicit ref-returns of shared Variables (partial…

02fd0cd

… fix) This is okay because we are passing a pointer and not the actual value. Currently excludes `ref` inferred from `auto ref`.

MoonlightSentinel force-pushed the ref_shared branch from 59a1aa0 to 02fd0cd Compare July 2, 2020 11:54

MoonlightSentinel approved these changes Jul 2, 2020

View reviewed changes

MoonlightSentinel added the Merge:auto-merge label Jul 2, 2020

MoonlightSentinel removed the Merge:auto-merge label Jul 2, 2020

thewilsonator merged commit 0b7a0d4 into dlang:master Jul 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow ref-returns of shared Variables #10412

Allow ref-returns of shared Variables #10412

UplinkCoder commented Sep 16, 2019

dlang-bot commented Sep 16, 2019 •

edited

Loading

TurkeyMan Sep 16, 2019

UplinkCoder Sep 16, 2019

MoonlightSentinel Jun 6, 2020

MoonlightSentinel Jun 6, 2020 •

edited

Loading

MoonlightSentinel Jun 6, 2020

TurkeyMan Jul 2, 2020

12345swordy commented Sep 16, 2019

TurkeyMan commented Sep 16, 2019

thewilsonator commented Sep 17, 2019

TurkeyMan commented Sep 17, 2019

thewilsonator commented Oct 19, 2019

thewilsonator commented Jun 1, 2020

UplinkCoder commented Jun 6, 2020

UplinkCoder commented Jun 6, 2020 via email

UplinkCoder commented Jun 6, 2020 via email

UplinkCoder commented Jun 6, 2020

thewilsonator commented Jun 6, 2020

UplinkCoder commented Jun 6, 2020

WalterBright commented Jun 6, 2020

WalterBright left a comment

MoonlightSentinel commented Jul 2, 2020

Geod24 commented Jul 2, 2020

Geod24 commented Sep 10, 2020

Allow ref-returns of shared Variables #10412

Allow ref-returns of shared Variables #10412

Conversation

UplinkCoder commented Sep 16, 2019

dlang-bot commented Sep 16, 2019 • edited Loading

Bugzilla references

Testing this PR locally

TurkeyMan Sep 16, 2019

Choose a reason for hiding this comment

UplinkCoder Sep 16, 2019

Choose a reason for hiding this comment

MoonlightSentinel Jun 6, 2020

Choose a reason for hiding this comment

MoonlightSentinel Jun 6, 2020 • edited Loading

Choose a reason for hiding this comment

MoonlightSentinel Jun 6, 2020

Choose a reason for hiding this comment

TurkeyMan Jul 2, 2020

Choose a reason for hiding this comment

12345swordy commented Sep 16, 2019

TurkeyMan commented Sep 16, 2019

thewilsonator commented Sep 17, 2019

TurkeyMan commented Sep 17, 2019

thewilsonator commented Oct 19, 2019

thewilsonator commented Jun 1, 2020

UplinkCoder commented Jun 6, 2020

UplinkCoder commented Jun 6, 2020 via email

UplinkCoder commented Jun 6, 2020 via email

UplinkCoder commented Jun 6, 2020

thewilsonator commented Jun 6, 2020

UplinkCoder commented Jun 6, 2020

WalterBright commented Jun 6, 2020

WalterBright left a comment

Choose a reason for hiding this comment

MoonlightSentinel commented Jul 2, 2020

Geod24 commented Jul 2, 2020

Geod24 commented Sep 10, 2020

dlang-bot commented Sep 16, 2019 •

edited

Loading

MoonlightSentinel Jun 6, 2020 •

edited

Loading