Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Supplemental fix for issue 13783 in std.parallelism #2773

Merged
merged 1 commit into from Dec 2, 2014

Conversation

9rnsr
Copy link
Contributor

@9rnsr 9rnsr commented Nov 29, 2014

auto f(ref ubyte); should not take an lvalue of enum E : ubyte.
Fixing 13783 will disallow the accepts-invalid bug.

`auto f(ref ubyte);` should not take an lvalue of `enum E : ubyte`.
Fixing 13783 will disallow the accepts-invalid bug.
@9rnsr
Copy link
Contributor Author

9rnsr commented Nov 29, 2014

Ready to merge. This PR is necessary for the compiler fix: dlang/dmd#4177

@quickfur
Copy link
Member

What makes this change necessary? Would similar user code also break as a result of the dmd PR?

@9rnsr
Copy link
Contributor Author

9rnsr commented Nov 30, 2014

Reduced code to show the bug in current std.parallelism module.

enum E : ubyte
{
    a,  // 0
    b,  // 1
    c   // 2
}

void invalidSet(ref ubyte stuff)
{
    stuff = 10;
}

void main()
{
    E e = E.a
    invalidSet(e);  // oops
    // e will be modified to a value out of the range of E without any casts.
    // it's a hole in type system.
}

The invalidSet call has been wrongly accepted by the compiler bug, but it will be disallowed correctly.
Yes, the dmd fix will stop to compile broken user code.

@quickfur
Copy link
Member

I see. But now we're making them template functions just to work around this problem? Wouldn't that introduce template bloat?

@9rnsr
Copy link
Contributor Author

9rnsr commented Nov 30, 2014

To reduce template bloat, we could replace those private functions with the direct calls of core.atomic functions. But, it would introduce casts everywhere. The std.parallelism author doesn't like that. (See the code comment from line 197).

@quickfur
Copy link
Member

quickfur commented Dec 1, 2014

Autotester failure on Win32, not sure if it's related to this PR or just another one of those heisenfailures.

@9rnsr
Copy link
Contributor Author

9rnsr commented Dec 1, 2014

It's definitely unrelated.

@quickfur
Copy link
Member

quickfur commented Dec 1, 2014

OK.

@quickfur
Copy link
Member

quickfur commented Dec 2, 2014

Auto-merge toggled on

quickfur pushed a commit that referenced this pull request Dec 2, 2014
Supplemental fix for issue 13783 in std.parallelism
@quickfur quickfur merged commit ca2171d into dlang:master Dec 2, 2014
@9rnsr
Copy link
Contributor Author

9rnsr commented Dec 2, 2014

Thanks!

@9rnsr 9rnsr deleted the fix13783 branch December 2, 2014 06:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants