Closed Bug 1259248 Opened 8 years ago Closed 8 years ago

Add an ArrayView class

Categories

(Core :: Graphics, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: jrmuizel, Unassigned)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch array-view (obsolete) — Splinter Review
This is a quick and dirty implementation so that we have something to iterate on.
Attachment #8734119 - Flags: review?(botond)
Attachment #8734119 - Attachment is patch: true
Attached patch array-viewSplinter Review
Attachment #8734119 - Attachment is obsolete: true
Attachment #8734119 - Flags: review?(botond)
Attachment #8734132 - Flags: review?(botond)
Comment on attachment 8734132 [details] [diff] [review]
array-view

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

LGTM as a first pass at a utility that we can start using right away. We should iterate on this (preferably matching the interface of one of the array_view-like classes being proposed for standardization) and move it to MFBT as a follow-up.

::: gfx/src/ArrayView.h
@@ +12,5 @@
> +
> +template<typename T>
> +class ArrayView
> +{
> +    public:

nits: use two-space identation, and access-specifiers don't get indented by an extra level

@@ +17,5 @@
> +        ArrayView(nsTArray<T>& aData) :
> +            mData(aData.Elements()), mLength(aData.Length())
> +        {
> +        }
> +        ArrayView(const T* aData, const size_t aLength) :

No need for the toplevel const on the second parameter

@@ +23,5 @@
> +        {
> +        }
> +        const T& operator[](const size_t aIdx) const
> +        {
> +            return mData[aIdx];

MOZ_ASSERT() that the index is in range
Attachment #8734132 - Flags: review?(botond) → review+
Comment on attachment 8734132 [details] [diff] [review]
array-view

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

Isn't this more or less the same as mfbt's Range class? https://dxr.mozilla.org/mozilla-central/source/mfbt/Range.h
https://hg.mozilla.org/mozilla-central/rev/d95bbecda3af
https://hg.mozilla.org/mozilla-central/rev/5ed70dba80c6
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.