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

Convert masses of HashSet<string> to BitSet<DamageType> #15400

Merged
merged 1 commit into from Jul 28, 2018

Conversation

Projects
None yet
5 participants
@chrisforbes
Copy link
Member

chrisforbes commented Jul 28, 2018

No description provided.

@chrisforbes chrisforbes requested review from pchote and MustaphaTR Jul 28, 2018

@pchote
Copy link
Member

pchote left a comment

LGTM and I didn't notice any regressions from limited ingame testing.

Just one minor nit that doesn't need to block merging.

@@ -47,7 +47,7 @@ class BridgeInfo : ITraitInfo, IRulesetLoaded, Requires<HealthInfo>, Requires<Bu
public WeaponInfo DemolishWeaponInfo { get; private set; }

[Desc("Types of damage that this bridge causes to units over/in path of it while being destroyed/repaired. Leave empty for no damage types.")]
public readonly HashSet<string> DamageTypes = new HashSet<string>();
public readonly BitSet<DamageType> DamageTypes = default(BitSet<DamageType>);

This comment has been minimized.

@pchote

pchote Jul 28, 2018

Member

Consistency nit: for BitSet<TargetableType> we left the default value implicit by not assigning anything, or in a couple of places using new BitSet<TargetType>(). It would be nice if we could be uniform, probably by changing the uses of BitSet<TargetType>.

This comment has been minimized.

@chrisforbes

chrisforbes Jul 28, 2018

Author Member

Not assigning anything produces warnings; standardize on default() ?

This comment has been minimized.

@pchote

pchote Jul 28, 2018

Member

Yes please.

@pchote pchote added the PR: Needs +2 label Jul 28, 2018

// Map strings to existing bits; do not allocate missing values new bits
BitSetIndex bits = 0;

lock (Bits)

This comment has been minimized.

@abcdefg30

abcdefg30 Jul 28, 2018

Member

Minor style nit: lock should also use braces as the inner block is using them.

This comment has been minimized.

@chrisforbes

chrisforbes Jul 28, 2018

Author Member

Done.

@chrisforbes chrisforbes force-pushed the chrisforbes:damagetype-bitset branch from 4e96bb6 to 215a5e0 Jul 28, 2018

@pchote

pchote approved these changes Jul 28, 2018

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Jul 28, 2018

👍 after Travis passes.

@pchote pchote merged commit d4ef841 into OpenRA:bleed Jul 28, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@@ -41,7 +40,7 @@ void INotifyKilled.Killed(Actor self, AttackInfo e)
if (IsTraitDisabled)
return;

if (Info.DeathTypes.Count == 0 || e.Damage.DamageTypes.Overlaps(Info.DeathTypes))
if (!Info.DeathTypes.IsEmpty || e.Damage.DamageTypes.Overlaps(Info.DeathTypes))

This comment has been minimized.

@reaperrr

reaperrr Jul 29, 2018

Contributor

This should've been Info.DeathTypes.IsEmpty (without '!'), if I'm reading this correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.