Skip to content

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