Closed Bug 1147766 Opened 5 years ago Closed 4 years ago

replace FixedStyleStructArray with a more re-usable class

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: heycam, Assigned: heycam)

References

Details

Attachments

(2 files, 3 obsolete files)

If we add a MinIndex template argument to mozilla::Array, we can replace FixedStyleStructArray with it.
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #8583646 - Flags: review?(jwalden+bmo)
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.
Blocks: 804975
And with an obvious bug fixed.
Attachment #8583646 - Attachment is obsolete: true
Attachment #8583646 - Flags: review?(jwalden+bmo)
Attachment #8583650 - Flags: review?(jwalden+bmo)
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?
Attachment #8583650 - Flags: review?(jwalden+bmo) → review-
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.
Flags: needinfo?(jwalden+bmo)
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.
Attachment #8583647 - Flags: review?(dbaron)
(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.)
Flags: needinfo?(jwalden+bmo)
I found a better name in the wild: RangedArray.
Attachment #8583647 - Attachment is obsolete: true
Summary: replace FixedStyleStructArray with mozilla::Array → replace FixedStyleStructArray with a more re-usable class
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().
Attachment #8585929 - Flags: review?(jwalden+bmo) → review+
(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?
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.
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;
Attachment #8609177 - Flags: review?(dbaron) → review+
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/12cf9fdc778f for annoying compilers by checking that an unsigned expression was >= 0.
(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
Flags: needinfo?(cam)
Sigh, sorry, I got the condition rewrite wrong.  Will back out.
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);
}
Thanks Jeff, luckily it looks like gcc is happy with the try run.
https://hg.mozilla.org/mozilla-central/rev/3cb74142cd09
https://hg.mozilla.org/mozilla-central/rev/5d5bd8ed66c9
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.