LT-22190 Use BitArray for large symbolic feature value sets

[AndyBlack]

This is a fix related to "L[T-22190 Hermit Crab adds wrong POSes to compound rules when there are more than 64 POSes"

The problem is that Machine uses a ulong to represent the possible values of a feature. This solution uses a BitArray instead of a ulong whenever there are more than sizeof(ulong) * 8 (=64) values.

The solution attempts to use composition instead of inheritance
.

---

This change is 

[codecov-commenter]

Codecov Report

❌ Patch coverage is 72.30216% with 77 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.28%. Comparing base (196ca8e) to head (dcfc257).

Files with missing lines	Patch %	Lines
.../FeatureModel/BitArraySymbolicFeatureValueFlags.cs	66.44%	43 Missing and 8 partials ⚠️
...ine/FeatureModel/UlongSymbolicFeatureValueFlags.cs	74.46%	13 Missing and 11 partials ⚠️
...c/SIL.Machine/FeatureModel/SymbolicFeatureValue.cs	93.75%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #330      +/-   ##
==========================================
+ Coverage   72.22%   72.28%   +0.06%     
==========================================
  Files         414      416       +2     
  Lines       35169    35388     +219     
  Branches     4871     4894      +23     
==========================================
+ Hits        25401    25581     +180     
- Misses       8676     8705      +29     
- Partials     1092     1102      +10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[ddaspit]

This is looking good. I just have a couple of comments. We should also see if we can optimize some of the methods that create create a lot of BitArray instances, such as ExceptWith. I think we can remove some of the extra instances creations.

@ddaspit reviewed 2 of 6 files at r1, all commit messages.
Reviewable status: 2 of 6 files reviewed, 2 unresolved discussions

---

src/SIL.Machine/FeatureModel/ISymbolicFeatureValue.cs line 5 at r1 (raw file):

namespace SIL.Machine.FeatureModel
{
    internal interface ISymbolicFeatureValue

Can we rename this to make it more clear how this is different than the SymbolicFeatureValue class? Maybe ISymbolicFeatureValueState. We should rename all of the associated classes as well.

---

src/SIL.Machine/FeatureModel/SymbolicFeature.cs line 52 at r1 (raw file):

        }

        internal ulong MaskUlong

We should pass the mask into the constructor of the ISymbolicFeatureValue implementation. It would also be nice to add an internal method to SymbolicFeature that creates the ISymbolicFeatureValue class. SymbolicFeatureValue would call it. That way you don't need to expose the mask properties.

[AndyBlack]

This is looking good. I just have a couple of comments. We should also see if we can optimize some of the methods that create create a lot of BitArray instances, such as ExceptWith. I think we can remove some of the extra instances creations.

Please make some suggestions of what this might be. I had problems getting some tests to pass until I made copies of these.

Can we rename this to make it more clear how this is different than the SymbolicFeatureValue class? Maybe ISymbolicFeatureValueState. We should rename all of the associated classes as well.

The classes that implement this interface are two different ways of handling the original _flags property, with one exception: SetFirst() that uses _flags to set the First property.

What do you think of changing the name to ISymbolicFeatueValueFlags or ISymbolicFeatueValueFlagsImpl?

src/SIL.Machine/FeatureModel/SymbolicFeature.cs line 52 at r1 (raw file):

        }

        internal ulong MaskUlong

We should pass the mask into the constructor of the ISymbolicFeatureValue implementation. It would also be nice to add an internal method to SymbolicFeature that creates the ISymbolicFeatureValue class. SymbolicFeatureValue would call it. That way you don't need to expose the mask properties.

I'm not following this yet. Maybe it will be clearer tomorrow...

[ddaspit]

Please make some suggestions of what this might be. I had problems getting some tests to pass until I made copies of these.

I will take a look and see if I can make more specific suggestions.

What do you think of changing the name to ISymbolicFeatueValueFlags or ISymbolicFeatueValueFlagsImpl?

I'm good with either of your name suggestions.

Reviewable status: 2 of 6 files reviewed, 2 unresolved discussions (waiting on @AndyBlack)

[Enkidu93]

Is this a duplicate/updated version of this PR?

[ddaspit]

I haven't done a good job of communicating my suggestions, so I will try making some of the changes and let you finish it up.

Reviewable status: 2 of 6 files reviewed, 2 unresolved discussions (waiting on @AndyBlack)

[AndyBlack]

I haven't done a good job of communicating my suggestions, so I will try making some of the changes and let you finish it up.

Sounds good. Thanks.

[AndyBlack]

@ddaspit Your changes are instructive and all look good to me. What is the proper etiquette here as far as approving the changes goes? Should I review them now? Or you? Or someone else?

[ddaspit]

@Enkidu93 Can you review my changes?

@ddaspit reviewed all commit messages.
Reviewable status: 0 of 10 files reviewed, 1 unresolved discussion (waiting on @AndyBlack)

[Enkidu93]

Yep! Sorry - I was out of town for a few days.

@Enkidu93 reviewed 10 of 10 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @AndyBlack)

[AndyBlack]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @AndyBlack)

[AndyBlack]

@ddaspit Eli approved your changes. I'm new to how this review system works. It looks like someone like you still needs to approve something. Is that right? If not, what is needed? Thanks.

[AndyBlack]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @AndyBlack)

[ddaspit]

@ddaspit reviewed 10 of 10 files at r2.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @AndyBlack)

[ddaspit]

@ddaspit reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @AndyBlack)