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

Document the behaviour of switches w.r.t 18712 #2512

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

thewilsonator
Copy link
Contributor

No description provided.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @thewilsonator! 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 annotated coverage diff directly on GitHub with CodeCov's browser extension
  • 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.

@thewilsonator
Copy link
Contributor Author

@n8sh can you approve this?

@n8sh
Copy link
Member

n8sh commented Nov 26, 2018

@thewilsonator I'll look it over.

$(GLINK CaseStatement), $(GLINK CaseRangeStatement), or $(GLINK DefaultStatement),
extending to the first appearance of one of
$(GLINK CaseStatement), $(GLINK CaseRangeStatement), $(GLINK DefaultStatement), `}`
or the end of a $(GLINK MixinStatement). One current limitation of this is that
Copy link
Member

Choose a reason for hiding this comment

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

What do you mean about the scope extending to the end of a mixin statement? It sounds to me like you're saying this code shouldn't work because x is out of scope at writeln(x), but it works:

https://run.dlang.io/is/cSGXa6

void main(string[] args)
{
    import std.stdio;

    switch (args.length)
    {
    case 1:
        int x;
        mixin("x += 2;");
        writeln(x);
        break;
    default:
        writeln("default");
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps that should be case labels, as

void main(string[] args)
{
    import std.stdio;

    switch (args.length)
    {
    mixin("case 1:");
        int x;
        x += 2;
        writeln(x);
        break;
    default:
        writeln("default");
    }
}

doesn't work.

Copy link
Member

Choose a reason for hiding this comment

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

This also fails with the same error message:

void main(string[] args)
{
    import std.stdio;

    switch (args.length)
    {
    case 1:
        int x;
        x += 2;
        mixin("writeln(x);");
    mixin("default:");
        writeln("default");
    }
}

So the end of a mixin statement definitely doesn't close a case label's implicit scope.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Huh. So the advice should be "don't mixin a case label".

Copy link
Member

Choose a reason for hiding this comment

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

I think the problem is that a mixed-in case label neither starts a new scope nor closes the previous case label's scope. This works:

void main(string[] args)
{
    import std.stdio;

    switch (args.length)
    {
    case 1:
        int x;
        x += 2;
        writeln(x);
        break;
    case 2:
        writeln("2");
        break;
    mixin("default:");
        writeln("default");
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wow. I'll have to think a bit more about how to word this. Thanks!

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