1434838 - New type `CheckedBool` could be used as result of tests on CheckedInts

Status: RESOLVED WONTFIX

(Core :: MFBT, enhancement)

Description

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Currently tests on CheckedInts are limited, and can inconspicuously escape the "Checked" domain:
- == is the only comparison possible, and it arbitrarily returns false if one CheckedInt is invalid, and possibly-confusingly it returns false when both are in the same invalid state.
- There is no != < <= > >=

So we end up with things like:
> // Explicit validity-check and conversion to unchecked before !=
> if (!ci1.isValid() || ci1.value() != 255) { ...
> 
> // Non-obvious use of CheckedInt::operator==, which returns a
> // bool with ambiguous invalid-state handling
> if (ci2 == i) { ...


I would propose that we create a new type with a clear interface, to be used as result of tests on CheckedInts, and that forces the user to visibly deal with valid/invalid situations:
> class CheckedBool
> {
>   MOZ_IMPLICIT CheckedBool(bool); // valid with value
>   CheckedBool(); // invalid
>   // Only 3 states possible:
>   bool isValidAndTrue() const;
>   bool isValidAndFalse() const;
>   bool isInvalid() const;
>   // Extra tests for clarity:
>   bool isInvalidOrFalse() const; // Clearer than !isValidAndTrue()
>   bool isInvalidOrTrue() const; // Clearer than !isValidAndFalse()
>   bool isValid() const; // Needed?
>   // + optional overloads: == != ! && ||
> };

We could implement all tests on CheckedInts: == != < <= > >=, they would now return a CheckedBool, forcing the user to clearly deal with invalid states.
E.g., the above example becomes:
> // Test result is checked, to compile we must explicitly test for expected state
> if ((ci != 255).isInvalidOrTrue()) { ...
> 
> // Clear handling of validity&value, making CheckedInt test more obvious
> if ((ci == i).isValidAndTrue()) { ...


Thoughts?

Comment 1

[Jeff Walden [:Waldo]]

That seems basically reasonable to me.

Comment 2

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1434838 - CheckedBool contains a bool with poisoned invalid state - r?jwalden

Its main use is as result of boolean operators on CheckedInts.

Comment 3

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa60aa6c04fb79ac754c32858b46f4beb67f0b50

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Thank you Jeff for the review.

(In reply to Jeff Walden [:Waldo] from https://phabricator.services.mozilla.com/D5522#inline-23044)
> While we can define these binary-test operators [on CheckedInts], on second look they seem a bit cute.
> Better to just make people clearly call isValid() first, then call value() for their actual test,
> clearly written. Let's not do these.

That was the motivation for CheckedBool. E.g., from comment 0:
> if ((ci != 255).isInvalidOrTrue()) { ...
which I thought was clearer than:
> if (!ci1.isValid() || ci1.value() != 255) { ...

I can't think of another good contextual use of CheckedBool (that couldn't be done with a Maybe<bool>).
No point continuing then.