Skip to content

Switch expressions #640

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

Open
wants to merge 14 commits into
base: dev
Choose a base branch
from
Open

Conversation

Daniel-Cortez
Copy link
Contributor

@Daniel-Cortez Daniel-Cortez commented Mar 14, 2021

What this PR does / why we need it:

This PR implements switch expressions, as proposed in #496.
Examples of use:

GetDiscount(numitems)
{
    assert(numitems > 0);
    return switch (numitems;
        1, 2, 3: 0;  // 1-3 items - full price
        4..10:   10; // 4-10 items - 10% off
        _:       15; // >10 items - 15% off
    );
}
GetWeaponSlot(weaponid)
{
    return switch (weaponid;
        0, WEAPON_BRASSKNUCKLE:
            0;
        WEAPON_GOLFCLUB, WEAPON_NITESTICK, WEAPON_KNIFE, WEAPON_BAT,
        WEAPON_SHOVEL, WEAPON_POOLSTICK, WEAPON_KATANA, WEAPON_CHAINSAW:
            1;
        WEAPON_COLT45, WEAPON_SILENCED, WEAPON_DEAGLE:
            2;
        WEAPON_SHOTGUN, WEAPON_SAWEDOFF, WEAPON_SHOTGSPA:
            3;
        WEAPON_UZI, WEAPON_MP5, WEAPON_TEC9:
            4;
        WEAPON_AK47, WEAPON_M4:
            5;
        WEAPON_RIFLE, WEAPON_SNIPER:
            6;
        WEAPON_ROCKETLAUNCHER, WEAPON_HEATSEEKER, WEAPON_FLAMETHROWER, WEAPON_MINIGUN:
            7;
        WEAPON_GRENADE, WEAPON_TEARGAS, WEAPON_MOLTOV, WEAPON_SATCHEL:
            8;
        WEAPON_SPRAYCAN, WEAPON_FIREEXTINGUISHER, WEAPON_CAMERA:
            9;
        WEAPON_DILDO, WEAPON_DILDO2, WEAPON_VIBRATOR, WEAPON_VIBRATOR2,
        WEAPON_FLOWER, WEAPON_CANE:
            10;
        WEAPON_NIGHT_VISION, WEAPON_THERMAL_VISION, WEAPON_PARACHUTE:
            11;
        WEAPON_BOMB:
            12;
        _:  -1;
    );
}

Which issue(s) this PR fixes:

Fixes #496

What kind of pull this is:

  • A Bug Fix
  • A New Feature
  • Some repository meta (documentation, etc)
  • Other

Additional Documentation:

@Y-Less
Copy link
Member

Y-Less commented May 2, 2021

FINALLY got around to doing some merging. I'm 90% happy with this, but still not sure about the default syntax. I really think it should do away with both the _: and the ; always. So your example becomes:

GetDiscount(numitems)
{
    assert(numitems > 0);
    return switch (numitems;
        1, 2, 3: 0;  // 1-3 items - full price
        4..10:   10; // 4-10 items - 10% off
        15 // >10 items - 15% off
    );
}

That to me more nicely mirrors the for syntax, and makes it more explicit that a default is required, as it must surely always be here.

@Daniel-Cortez
Copy link
Contributor Author

Daniel-Cortez commented May 2, 2021

Forgot to mention, the last ; (the one after the default) is optional. I'm not sure if removing the _: part completely would be a good idea, but I guess I can make it optional as well.

@Y-Less
Copy link
Member

Y-Less commented May 2, 2021

I'm good with both optional.

@Y-Less
Copy link
Member

Y-Less commented May 2, 2021

Forgot to mention, the last ; (the one after the default) is optional. I'm not sure if removing the _: part completely would be a good idea, but I guess I can make it optional as well.

Actually, there is an issue with that:

new Tag:a;

new b = switch(thing;
    5: 6;
    _:a
);

Now becomes ambiguous - is that an explicit default, or an implicit default with a tag override on the value? I know I asked about tagged values in the cases, but what about in the values?

@Daniel-Cortez
Copy link
Contributor Author

It's an explicit default. If you want an implicit one with a tag override, you can enclose the expression into parentheses.

return switch (value;
    1: 2;
       (_:TaggedResult())
);

@Y-Less
Copy link
Member

Y-Less commented May 3, 2021

I'm not sure if the optional _: is in the latest version of the PR, but this code gives a staging buffer overflow:

#include <a_samp>

Five()
{
	printf("5");
}

Six()
{
	printf("6");
}

main()
{
	new a = 5;
	new b = switch (a;
		5: Five();
		6: Six();
		7
	);
	printf("%d", b);
}

Also, when I change it to an explicit default:

#include <a_samp>

Five()
{
	printf("5");
}

Six()
{
	printf("6");
}

main()
{
	new a = 5;
	new b = switch (a;
		5: Five();
		6: Six();
		_: 7;
	);
	printf("%d", b);
}

I get no warnings, which is not good, because there should be warnings that Five and Six should return values.

@Y-Less
Copy link
Member

Y-Less commented May 3, 2021

I tried without a default at all:

#include <a_samp>

Five()
{
	printf("5");
	return 2;
}

Six()
{
	printf("6");
	return 1;
}

main()
{
	new a = 7;
	new b = switch (a;
		5: Five();
		6: Six();
	);
	printf("%d", b);
}

Firstly, that code took a weirdly long time to compile, it should be almost instant, but I was waiting a good few seconds. Then there's no error for missing defaults. If this is by design the fallbacks need to be documented, but I don't think they should be optional in an expression. This code actually printed 7, which I wasn't expecting.

@Daniel-Cortez
Copy link
Contributor Author

Daniel-Cortez commented May 4, 2021

I'm not sure if the optional _: is in the latest version of the PR, but this code gives a staging buffer overflow

Thanks. I'm already aware of this problem and fixed it locally yesterday, just didn't finish the tests, which is why I haven't uploaded the fix yet.

I tried without a default at all

Firstly, that code took a weirdly long time to compile, it should be almost instant, but I was waiting a good few seconds. Then there's no error for missing defaults. If this is by design the fallbacks need to be documented, but I don't think they should be optional in an expression.

No, this is not by design. That's strange; even though I wrote most of the code a year and a half ago, I clearly remember making a check for a default branch. Maybe I implemented it in a separate experimental branch and didn't merge it into the main feature branch or something...

EDIT: Indeed, it was in a separate branch; not sure why I didn't merge it in - probably because it adds a new error code for situations when a switch expression doesn't contain a default case.
Anyway, I'll upload all the changes in a few hours, when I finish with the tests.

@stale
Copy link

stale bot commented Apr 16, 2022

This issue has been automatically marked as stale because it has not had recent activity.

@stale stale bot added the state: stale label Apr 16, 2022
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.

2 participants