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

Add support for shortened switch #20658

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

bangbangsheshotmedown
Copy link
Contributor

@bangbangsheshotmedown bangbangsheshotmedown commented Jan 8, 2025

clean and smooth

enum K = 42;
switch (K)
{
    case 42 => printf("yes!\n");

    default => printf("no!\n");
}

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @bangbangsheshotmedown! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

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 + dmd#20658"

@thewilsonator
Copy link
Contributor

This change will require a DIP. Please post the idea to https://forum.dlang.org/group/dip.ideas before spending too much time here.

@bangbangsheshotmedown
Copy link
Contributor Author

bangbangsheshotmedown commented Jan 8, 2025

This change will require a DIP. Please post the idea to https://forum.dlang.org/group/dip.ideas before spending too much time here.

You didn't need a DIP for #20653, why should I?

@ntrel
Copy link
Contributor

ntrel commented Jan 8, 2025

Again, this would need to be discussed on the forum or at the very least an issue filed for discussion. I think it's best to keep pull request comments focused on implementation issues. Note that pattern matching is planned, so IMO tweaking switch/case is unlikely to be accepted.

@thewilsonator
Copy link
Contributor

You didn't need a DIP for #20653, why should I?

That will need a DIP at some point.

@Herringway
Copy link
Contributor

You didn't need a DIP for #20653, why should I?

You need to write a DIP so the foundation can unofficially reject it (by ignoring it) while they're busy copying C and C++ features. That's important work and mustn't be interrupted by annoying formalities.

@nordlow
Copy link
Contributor

nordlow commented Jan 9, 2025

I believe -> should be replaced with => for conformance with existing shortened function (method) syntax.

@atilaneves
Copy link
Contributor

I don't know how common it is to have one statement for each case. It is common however to have a one-line return, but that's already clean and smooth:

enum K = 42;
switch (K)
{
    case 42: return 0;
    default: return 1;
}

@WalterBright
Copy link
Member

The proposal needs some clarification:

switch (K)
{
    case 42 => printf("yes!\n");
    default => printf("no!\n");
}

should rewrite to:

switch (K)
{
    case 42: { printf("yes!\n"); break; }
    default: { printf("no!\n"); break; }
}

to make the issue of variable scopes clear.

@bangbangsheshotmedown
Copy link
Contributor Author

bangbangsheshotmedown commented Jan 15, 2025

@WalterBright I have no idea how to do that, I'm sorry

I'll need someone to help me

@bangbangsheshotmedown
Copy link
Contributor Author

the issue of variable scopes

Can you please share a code example? I'm not sure what to test exactly

@ntrel
Copy link
Contributor

ntrel commented Jan 15, 2025

switch (K)
{
    case 42 => int x;
    default => x++; // should error, `x` not found
}

@bangbangsheshotmedown
Copy link
Contributor Author

bangbangsheshotmedown commented Jan 15, 2025

@ntrel

I guess the code already handles it, because it doesn't compile:

Error: undefined identifier `x`
        default => x++;

@ntrel
Copy link
Contributor

ntrel commented Jan 15, 2025

You're right, the scopes don't overlap. Perhaps @WalterBright has another example?

@WalterBright
Copy link
Member

Duff's Device:

void send(short* to, short* from, int count)
{
    auto n = (count + 7) / 8;
    switch (count % 8) {
    case 0: do { *to = *from++;
                 goto case;
    case 7:      *to = *from++;
                 goto case;
    case 6:      *to = *from++;
                 goto case;
    case 5:      *to = *from++;
                 goto case;
    case 4:      *to = *from++;
                 goto case;
    case 3:      *to = *from++;
                 goto case;
    case 2:      *to = *from++;
                 goto case;
    case 1:      *to = *from++;
                 continue;
            } while (--n > 0);
            break;
    default: break;
    }
}

The do introduces a scope that spans some of the case statements.

@Herringway
Copy link
Contributor

It's unfortunate that it's too late to forbid such painful-to-read pessimizations like duff's device in D...

@bangbangsheshotmedown
Copy link
Contributor Author

Duff's Device:

void send(short* to, short* from, int count)
{
    auto n = (count + 7) / 8;
    switch (count % 8) {
    case 0: do { *to = *from++;
                 goto case;
    case 7:      *to = *from++;
                 goto case;
    case 6:      *to = *from++;
                 goto case;
    case 5:      *to = *from++;
                 goto case;
    case 4:      *to = *from++;
                 goto case;
    case 3:      *to = *from++;
                 goto case;
    case 2:      *to = *from++;
                 goto case;
    case 1:      *to = *from++;
                 continue;
            } while (--n > 0);
            break;
    default: break;
    }
}

The do introduces a scope that spans some of the case statements.

this one too works

void send_1(short* to, short* from, int count)
{
    auto n = (count + 7) / 8;
    switch (count % 8) {
    case 0: do { *to = *from++;
                 goto case;
    case 7:      *to = *from++;
                 goto case;
    case 6:      *to = *from++;
                 goto case;
    case 5:      *to = *from++;
                 goto case;
    case 4:      *to = *from++;
                 goto case;
    case 3:      *to = *from++;
                 goto case;
    case 2:      *to = *from++;
                 goto case;
    case 1:      *to = *from++;
                 continue;
            } while (--n > 0);
            break;
    default: break;
    }
}

void send_2(short* to, short* from, int count)
{
    auto n = (count + 7) / 8;
    switch (count % 8) {
    case 0 => do { *to = *from++;
                 goto case;
    case 7 =>      *to = *from++;
                 goto case;
    case 6 =>      *to = *from++;
                 goto case;
    case 5 =>      *to = *from++;
                 goto case;
    case 4 =>      *to = *from++;
                 goto case;
    case 3 =>      *to = *from++;
                 goto case;
    case 2 =>      *to = *from++;
                 goto case;
    case 1 =>      *to = *from++;
                 continue;
            } while (--n > 0);
            break;
    default: break;
    }
}

Granted the test case is valid:

    {
        short[4] data = [1,2,3,4];
        short[4] output = 0;
        send_1(output.ptr, data.ptr, cast(int) data.length);
        assert(output[0] == 1 && output[1] == 2 && output[2] == 3 && output[3] == 4);
    }
    {
        short[4] data = [1,2,3,4];
        short[4] output = 0;
        send_2(output.ptr, data.ptr, cast(int) data.length);
        assert(output[0] == 1 && output[1] == 2 && output[2] == 3 && output[3] == 4);
    }

@bangbangsheshotmedown
Copy link
Contributor Author

@WalterBright

There was a mistake in your code, should be: *to++

@bangbangsheshotmedown
Copy link
Contributor Author

I checked with dmd -vcg-ast

    enum K = 42;
    switch (K)
    {
        case 42 => printf("yes!\n");

        default => printf("no!\n");
    }

becomes:

    enum int K = 42;
    switch (42)
    {
        case 42:
        {
            printf("yes!\n");
            break;
        }
        default:
        {
            printf("no!\n");
            break;
        }
    }

@pbackus
Copy link
Contributor

pbackus commented Jan 19, 2025

  1. I don't think it's worth adding an entirely new syntax just to avoid writing break;
  2. We may want to use case ... => for pattern matching in the future, the way Java does.

@ichordev
Copy link

  1. I don't think it's worth adding an entirely new syntax just to avoid writing break;
  2. We may want to use case ... => for pattern matching in the future, the way Java does.
  1. True
  2. It looks like this syntax should be a switch expression. We could later adapt it into pattern matching.
    Switch expression:
auto a = switch(b){
    case x => 1;
    case y => 3;
    default => 7;
}

Pattern matching:

auto a = switch(sum){
    case Vec2(x, y) => x+y;
    case string c => c.to!int();
    default => { assert(0); };
}

Essentially the same as this: https://forum.dlang.org/thread/[email protected]

@bangbangsheshotmedown
Copy link
Contributor Author

  1. I don't think it's worth adding an entirely new syntax just to avoid writing break;

why did you add shortened functions syntax only just to avoid writing {}

if you want to shit on features, please do it consistently

  1. We may want to use case ... => for pattern matching in the future, the way Java does.

there is no such thing in D today, please stick to what exist today, so we can improve things today

i'm not interested in arguing for 10 years, eg: tuples DIP

@bangbangsheshotmedown
Copy link
Contributor Author

  1. I don't think it's worth adding an entirely new syntax just to avoid writing break;
  2. We may want to use case ... => for pattern matching in the future, the way Java does.
  1. True
  2. It looks like this syntax should be a switch expression. We could later adapt it into pattern matching.
    Switch expression:
auto a = switch(b){
    case x => 1;
    case y => 3;
    default => 7;
}

Pattern matching:

auto a = switch(sum){
    case Vec2(x, y) => x+y;
    case string c => c.to!int();
    default => { assert(0); };
}

Essentially the same as this: https://forum.dlang.org/thread/[email protected]

i am working on a PR for switch as expression, let's get this in first

@bangbangsheshotmedown
Copy link
Contributor Author

to people who object, i want to see your PRs for pattern matching, talk is cheap

@ichordev
Copy link

to people who object, i want to see your PRs for pattern matching, talk is cheap

They would never be merged because we have no DIP and not everyone is a compiler dev. I’m waiting for Rikki’s match statement DIP (which I linked above) to have a final decision made. It has been some months so there must’ve been some delays. If Rikki’s DIP is accepted, then we can just use that instead of adding a very similar feature with slightly different syntax.

@ntrel
Copy link
Contributor

ntrel commented Jan 20, 2025

why did you add shortened functions syntax only just to avoid writing {}

Actually today => expr; means { return expr; }, where expr is an expression only. With this pull, you have changed the meaning to allow => Statement but only for case statements. People will then ask why they can't do => Statement for a function body - that is currently not supported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.