Skip to content

Conversation

@quickfur
Copy link
Member

No description provided.

@quickfur quickfur requested a review from CyberShadow as a code owner March 30, 2020 22:33
@dlang-bot
Copy link
Contributor

Thanks for your pull request, @quickfur!

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 run digger -- build "master + phobos#7433"

@CyberShadow
Copy link
Member

Hi & thanks for the PR,

As with past attempts to improve the Socket methods' attributes, I think this should be a breaking change, as making receive @nogc in Socket would mean that all classes that subclass Socket would now need to have their receive also @nogc. We have a test for this but for some reason it is not triggered by this change. What am I missing?

@schveiguy
Copy link
Member

Why would that test trigger? asserts are @nogc.

@CyberShadow
Copy link
Member

CyberShadow commented Mar 30, 2020

We have a test for this but for some reason it is not triggered by this change. What am I missing?

OK, I see the compiler is assuming that the method in the derived class is @nogc , which our stub method in the test is, but that may not be true for the real cases which the test is supposed to represent, which means the test is broken. I will submit a fix.

Why would that test trigger? asserts are @nogc.

Because the method in the derived class was not annotated @nogc. (I forgot that method annotations implicitly propagate to derived classes.)

@schveiguy
Copy link
Member

And I agree, this should be considered a breaking change.

@CyberShadow
Copy link
Member

RFC: #7434

@n8sh
Copy link
Member

n8sh commented Apr 26, 2020

There is a previous non-accepted PR identical to this one, Add @nogc attribute to Socket receive methods (#6730).

@pinver
Copy link

pinver commented Apr 26, 2020

It's not "not accepted", but pending for a decision: note the "label" and the discussion.

@n8sh
Copy link
Member

n8sh commented Apr 26, 2020

Perhaps "open" would have been a better term than "non-accepted."

@quickfur
Copy link
Member Author

I don't have the time / patience to work on this any more. Somebody else please pick this up.

@quickfur quickfur closed this Jul 30, 2020
@thewilsonator thewilsonator added the Review:Phantom Zone Has value/information for future work, but closed for now label Jul 31, 2020
@pinver
Copy link

pinver commented Jul 31, 2020

@thewilsonator , as stated above, #7434 exactly the same issue, with a little more details: I think this can simply be closed.

@thewilsonator thewilsonator removed the Review:Phantom Zone Has value/information for future work, but closed for now label Jul 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants