1147766 - replace FixedStyleStructArray with a more re-usable class

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Cameron McCormack (:heycam)]

If we add a MinIndex template argument to mozilla::Array, we can replace FixedStyleStructArray with it.

Comment 1

[Cameron McCormack (:heycam)]

Attachment: Part 1: Support mozilla::Arrays with a non-zero index base.

Comment 2

[Cameron McCormack (:heycam)]

Attachment: Part 2: Replace FixedStyleStructArray with mozilla::Array.

Comment 3

[Cameron McCormack (:heycam)]

In bug 804975 I want to experiment with putting the next-style-struct pointer and the cache key in nsRuleNode::mStyleData, so this will make it simpler to use different types for the inherit and reset struct arrays.

Comment 4

[Cameron McCormack (:heycam)]

Attachment: Part 1: Support mozilla::Arrays with a non-zero index base.

And with an obvious bug fixed.

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 8583650 [details] [diff] [review]
Part 1: Support mozilla::Arrays with a non-zero index base.

Review of attachment 8583650 [details] [diff] [review]:
-----------------------------------------------------------------

I should have made this clear in a comment, but mozilla::Array is basically a stand-in for std::array in <array> (or at least a subset of it, growing as needed), so we're not really free to change it this way.  Can you use it via encapsulation somehow instead?

Comment 6

[Cameron McCormack (:heycam)]

Ah, I didn't realise that.  Would you accept a new class in mfbt/, something like mozilla::NonZeroBasedArray (suggestion for better name welcome)?  Or would you rather that be defined elsewhere.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8583647 [details] [diff] [review]
Part 2: Replace FixedStyleStructArray with mozilla::Array.

Clearing review until you sort out the MFBT side, although the idea seems fine.

Comment 8

[Jeff Walden [:Waldo]]

(In reply to Cameron McCormack (:heycam) from comment #6)
> Would you accept a new class in mfbt/, something like
> mozilla::NonZeroBasedArray (suggestion for better name welcome)?

Seems reasonable enough.  (The idea, not the name.  I don't have ideas for the name.)

Comment 9

[Cameron McCormack (:heycam)]

I found a better name in the wild: RangedArray.

Comment 10

[Cameron McCormack (:heycam)]

Attachment: Part 1: Add a mozilla::RangedArray class, for fixed length arrays with a non-zero base index.

Comment 11

[Jeff Walden [:Waldo]]

Comment on attachment 8585929 [details] [diff] [review]
Part 1: Add a mozilla::RangedArray class, for fixed length arrays with a non-zero base index.

Review of attachment 8585929 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/RangedArray.h
@@ +4,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/* A compile-time constant-length array with bounds-checking assertions
> +   and support for a non-zero base. */

"support for a non-zero base" is really not clear to me, not without reading the implementation, and once you have to do that you've lost.  Also a minor style nit.  Cumulatively, let's do this instead:

/**
 * A compile-time constant-length array, with bounds-checking assertions -- but
 * unlike mozilla::Array, with indexes biased by a constant.
 *
 * Thus where mozilla::Array<int, 3> is a five-element array indexed by [0, 3),
 * mozilla::RangedArray<int, 8, 3> is a five-element array indexed by [8, 11).
 */

@@ +28,5 @@
> +    return mArr[aIndex - MinIndex];
> +  }
> +
> +private:
> +  Array<T, Length> mArr;

You're bootlegging the #include for this.  Add an explicit #include "mozilla/Array.h" just beneath the include-guard.

@@ +33,5 @@
> +};
> +
> +} // namespace mozilla
> +
> +#endif // !defined(mozilla_RangedArray_h)

Normal style just repeats the include-guard define, not !defined().

Comment 12

[Nathan Froyd [:froydnj]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #11)
>  * Thus where mozilla::Array<int, 3> is a five-element array indexed by [0,
> 3),
>  * mozilla::RangedArray<int, 8, 3> is a five-element array indexed by [8,
> 11).

Surely these are three-element arrays, not five-element arrays?

Bikeshed: makes slightly more sense to have RangedArray<T, Length, MinIndex>, I think, so that one can think of MinIndex being defaulted to 0?

Comment 13

[Jeff Walden [:Waldo]]

Er, yes, three-element, I changed my example and didn't update the words.

And yeah, I was kind of thinking maybe the other order was more sensible, but not particularly sure.  Other order's a good idea then.

Comment 14

[Cameron McCormack (:heycam)]

I think the (MinIndex, Length) order reads better, although I was prepared to put up with the order order if we were actually modifying mozilla::Array by adding a defaulted argument.  In the one place I want to use it (well, two places with identical usage), it will be:

  struct nsInhertedStyleData
  {
    mozilla::RangedArray<void*,
                         nsStyleStructID_Inherited_Start,
                         nsStyleStructID_Inherited_Count> mStyleStructs;
    ...
  }:

and I think that looks more sensible than

    mozilla::RangedArray<void*,
                         nsStyleStructID_Inherited_Count,
                         nsStyleStructID_Inherited_Start> mStyleStructs;

Comment 15

[Cameron McCormack (:heycam)]

Attachment: Part 2: Replace FixedStyleStructArray with mozilla::RangedArray.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5754455de846
https://hg.mozilla.org/integration/mozilla-inbound/rev/10d6fcd9a4e0

Comment 17

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/12cf9fdc778f for annoying compilers by checking that an unsigned expression was >= 0.

Comment 18

[Cameron McCormack (:heycam)]

(In reply to Phil Ringnalda (:philor) from comment #17)
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/12cf9fdc778f for
> annoying compilers by checking that an unsigned expression was >= 0.

"annoying compilers" sounds about right.

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

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a6fa096f9853
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec1c41143ff6

Comment 20

[Cameron McCormack (:heycam)]

Sigh, sorry, I got the condition rewrite wrong.  Will back out.

Comment 21

[Cameron McCormack (:heycam)]

Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/4ac52fce1beb
Try again:  https://treeherder.mozilla.org/#/jobs?repo=try&revision=baf8bcbc2152

Comment 22

[Jeff Walden [:Waldo]]

If that last try doesn't work, this may as well -- but with rather more template to it, so ideally not:

template<typename T,
         T Value,
         bool = mozilla::MinValue<T>::value == Value>
struct AssertGreaterThanOrEqual;

template<typename T, T Value>
struct AssertGreaterThanOrEqual<T, Value, true>
{
  static void run(T x)
  {
    // If the limit is the minimum value, there's nothing to assert.
  }
};

template<typename T, T Value>
struct AssertGreaterThanOrEqual<T, Value, false>
{
  static void run(T x)
  {
    MOZ_ASSERT(x >= Value);
  }
};

int main()
{
  AssertGreaterThanOrEqual<unsigned, 0>::run(0);
}

Comment 23

[Cameron McCormack (:heycam)]

Thanks Jeff, luckily it looks like gcc is happy with the try run.

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3cb74142cd09
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d5bd8ed66c9

Comment 25

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3cb74142cd09
https://hg.mozilla.org/mozilla-central/rev/5d5bd8ed66c9