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

Allow empty statements in switch #13

Merged
merged 5 commits into from
Feb 12, 2025
Merged

Conversation

arturjpv
Copy link
Member

@arturjpv arturjpv commented Feb 11, 2025

Allow empty statements in switch.

@arturjpv arturjpv requested a review from tvandijck as a code owner February 11, 2025 14:41
@arturjpv arturjpv requested a review from webbju February 11, 2025 14:42
@tvandijck
Copy link
Collaborator

is there a code example for this?

@arturjpv
Copy link
Member Author

arturjpv commented Feb 11, 2025

is there a code example for this?

From Pioneer\Script\Pioneer\Constructable\StyleDrivers\GlowingMetalStyleDriver.as

	UFUNCTION()
	private void OnCapabilityEventOccured(const UCapability Capability, const FConstructableCapabilityEvent& EventData)
	{
		if (Capability.AbilityTags.MatchesQuery(AbilityTagQuery))
		{
			switch(EventData.EventType)
			{
				case EConstructableCapabilityEventType::Stop:
					StartFadeOut();
					break;
				default:
			}
		}
	}

	UFUNCTION()
	private void OnCapabilityStateChanged(const UCapability Capability,
	                                      const FCapabilityStateData& StateData)
	{
		if (Capability.AbilityTags.MatchesQuery(AbilityTagQuery))
		{
			switch(StateData.State)
			{
				case ECapabilityState::Preparing:
					StartFadeIn();
					break;
				case ECapabilityState::CoolingDown:
					StartFadeOut();
					break;
				default:
			}
		}
	}

@tvandijck
Copy link
Collaborator

that looks like it's just bad code... I wouldn't accept that... people shouldn't check-in nonsense like that.

@arturjpv
Copy link
Member Author

arturjpv commented Feb 11, 2025

that looks like it's just bad code... I wouldn't accept that... people shouldn't check-in nonsense like that.

I agree it's bad code, but it compiles. And the server code exciser fails to parse it... so it's not even treated as an error. The file is skipped. So this missmatch betwen compiler and "server scope linter" can be bad.

We also have this one in Discovery\GameMode\GameShowEvents\AlienInvasion\AlienInvasionFlyingSaucer.as:

	void ChangeMovementMode(ESaucerMovementMode DesiredMovementMode)
	{
		switch (DesiredMovementMode)
		{
			case ESaucerMovementMode::Stalk:
			{
				CurrentMovementMode = ESaucerMovementMode::Stalk;
				TargetFlySpeed = StalkModeSpeed;
				return;
			}
			case ESaucerMovementMode::Loiter:
			{
				CurrentMovementMode = ESaucerMovementMode::Loiter;
				TargetFlySpeed = LoiterModeSpeed;
				return;
			}
			case ESaucerMovementMode::Boost:
			case ESaucerMovementMode::None:
		}
	}

Thaw will force us to add empty statements for any labeled case statement.

I will try to solve it this same PR... as probably one modification affects the other.

@arturjpv
Copy link
Member Author

that looks like it's just bad code... I wouldn't accept that... people shouldn't check-in nonsense like that.

The other option is two modify all those switch statements to make them good and don't modify the grammar. But then, maybe, we don't notice that people is submitting more code like that in other parts and we have more files skipping checks because of not being able to parse them.

@arturjpv arturjpv marked this pull request as draft February 11, 2025 15:03
@arturjpv arturjpv marked this pull request as ready for review February 11, 2025 15:16
@arturjpv arturjpv changed the title Allow empty default statement in switch Allow empty statements in switch Feb 11, 2025
Copy link
Member

@webbju webbju left a comment

Choose a reason for hiding this comment

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

I see your point here @arturjpv. The current code is syntactically valid despite how poor practice it may be. For me this is a 'lgtm' based on the premise that this tool is not a linter, and that this kind of coding style issue should probably be solved with compiler warning flags or other tooling.

@arturjpv arturjpv merged commit 7722e8f into main Feb 12, 2025
1 check passed
@arturjpv arturjpv deleted the feature/empty-default-statement branch February 12, 2025 10:10
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.

3 participants