Closed Bug 1309466 Opened 3 years ago Closed 3 years ago

Add support for initializer_list to Array and EnumeratedArray

Categories

(Core :: MFBT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: boris, Assigned: JamesCheng)

References

Details

Attachments

(2 files)

Use an initializer_list to initialize a fixed size array, instead of a for loop, just like what std library does.

e.g.
enum class MyType {
  A,
  B,
  C,
  Count
};
EnumeratedArray<MyType, MyType::Count, int> myType;

In order to initialize this EnmeratedArray, we have to do something like this:

for (auto&& i : myType) {
  i = 0;
}

If we support initializer_list for constructor, we can initialize it more concisely.

EnumeratedArray<MyType, MyType::Count, int> myType = { 0, 0, 0 };
It sounds a good way to initialize the Array.

Hi Boris, Could I take this bug?

Thank you.
Flags: needinfo?(boris.chiou)
Sure. :)
Flags: needinfo?(boris.chiou)
Assignee: nobody → jacheng
Assignee: jacheng → nobody
Assignee: nobody → jacheng
Attachment #8800137 - Flags: review?(nfroyd)
Can't we just do:

template <typename... Args> 
Array(Args... args)
  : mArr{args...}
{}

That makes it a compile error to specify an initializer list of the wrong length, and we don't need the MOZ_ASSERT.
Thanks for your advice.

Yes, the variadic constructor can allow (1),(2),(3) to initialize

(1)Array<int32_t, 3> hi(1,2,3);
(2)Array<int32_t, 3> hi2({1,2,3});
(3)Array<int32_t, 3> hi3 = {1,2,3};

But initializer_list cannot allow (1) to pass compiling...

So I will adopt your opinion.

Here is the treeherder result
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e82963ef5da8
It seems the static analysis determine the variadic constructor should also declared as MOZ_IMPLICIT

Try next round 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d719a74f36f5f4f5d787666b2ec3894eadc7f110&selectedJob=29144856
Hi glandium and Matt,

My patch will cause all windows machine with strange build errors...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8f2274cc21a&selectedJob=29156680

Do you have any idea(I tried lot of times)? I tried pushing without my patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4b7bd5359d5&selectedJob=29156614

It looks fine but I have no idea how the build error happened.

Hi Matt,

Do you mind I rollback to the original ```initializer_list``` version if I cannot fix the error? 

Thank you.
Flags: needinfo?(mh+mozilla)
Flags: needinfo?(matt.woodrow)
I think I found the root cause of the build error but I still have no idea why.

I modified ```Array()=default;``` into ```Array(){}```.

The compile error was fixed[1].

I thought Constructor()=default is equivalent to Constructor(){} but it seems not.

Hi Nathan,

Have you ever met it before or haven any experience on it?

Thank you.

Sorry for ni? so many people, I just want to know the reason why I met this issue.

[1]
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac7d145becda
Flags: needinfo?(nfroyd)
(In reply to James Cheng[:JamesCheng] from comment #12)
> I think I found the root cause of the build error but I still have no idea
> why.
> 
> I modified ```Array()=default;``` into ```Array(){}```.
> 
> The compile error was fixed[1].
> 
> I thought Constructor()=default is equivalent to Constructor(){} but it
> seems not.
> 
> Hi Nathan,
> 
> Have you ever met it before or haven any experience on it?

You are correct, they are supposed to be equivalent.  I haven't seen anything like that before.
Flags: needinfo?(nfroyd)
ok... there must have some bugs in MSVC.

Anyway, I can use {} instead of default.

Please help to review this patch

Thank you :)
Comment on attachment 8800137 [details]
Bug 1309466 - Enable Array and EnumeratedArray to be brace-initialized with initial contents.

https://reviewboard.mozilla.org/r/85148/#review83742

I'm skeptical about the need for this.  Nearly all `EnumeratedArray`s (and `Array`s, so far as I can tell) in the codebase are used as member variables, where there's no special initialization needed.

The lone exception, in dom/canvas/WebGLContextExtensions.cpp, could use this constructor, by having a file-scope `EnumeratedArray` instead of indirecting through `GetExtensionString`.  But then having a file-scope variable intialized with a non-trivial, non-constexpr constructor will, on some compilers, introduce static constructors, which we try to avoid.  Having this new constructor would encourage bad practices in the case where it is most likely to be used, and we'd have to have something like `GetExtensionString` anyway to avoid the static constructor.

`std::array` gets around this issue of static constructors because it doesn't have an `array(const initializer_list<T>&)` constructor: instead, the wrapped array is exposed as a public member, and normal brace-initialization rules for constructing aggregates apply, and compilers can generally compile those without introducing static constructors unexpectedly.  The standard can get away with that, I suppose, because you can't depend on the internal implementation details of `std::array`, so you shouldn't be referring to the public member by name.  We could do the same thing here, I guess, but we'd have to call the member something fear-inducing, like mDoNotTouchThisArray.

Do you have a patch or work-in-progress-patch where this makes things significantly shorter?

This change should include tests for being able to initialize an array via a brace-initializer.
Attachment #8800137 - Flags: review?(nfroyd)
(In reply to Nathan Froyd [:froydnj] from comment #16)
> Comment on attachment 8800137 [details]
> Bug 1309466 - Add support for initializer_list to Array and EnumeratedArray.
> 
> https://reviewboard.mozilla.org/r/85148/#review83742
> 
> I'm skeptical about the need for this.  Nearly all `EnumeratedArray`s (and
> `Array`s, so far as I can tell) in the codebase are used as member
> variables, where there's no special initialization needed.

There is a WIP may need this: Bug 1272549 Attachment 8800124 [details]:

Declare in nsStyleTransformMatrix.h:
enum class ShearType {
  XYSHEAR,
  XZSHEAR,
  YZSHEAR,
  Count
};
using ShearArray = mozilla::EnumeratedArray<ShearType, ShearType::Count, float>;

Usage in StyleAnimationValue.cpp:
If we don't support initialization, I have to initialize this array as:
ShearArray shear1;
for (auto&& s : shear1) {
  s = 0.0f;
}
// Then, we put *some* values into shear1 by other functions, so I'd like to initialize it first.

I just want to make it shorter, as initializing a primitive fixed array:
ShearArray shear1 = {0.0f, 0.0f, 0.0f};

Or do you have a better way to make it more concise without this initializer_list? Thanks.
Flags: needinfo?(nfroyd)
(In reply to Boris Chiou [:boris] from comment #17)
> (In reply to Nathan Froyd [:froydnj] from comment #16)
> > I'm skeptical about the need for this.  Nearly all `EnumeratedArray`s (and
> > `Array`s, so far as I can tell) in the codebase are used as member
> > variables, where there's no special initialization needed.
>
> I just want to make it shorter, as initializing a primitive fixed array:
> ShearArray shear1 = {0.0f, 0.0f, 0.0f};
> 
> Or do you have a better way to make it more concise without this
> initializer_list? Thanks.

Eh, if we had an equivalent of std::array::fill, we'd just have:

  ShearArray shear1;
  shear1.fill(0.0f);

Worth noting that we'd have to make sure that the initializer_list was the length we expect at runtime, and we'd need a test or two.  Might be easier to adopt std::array's approach described in comment 16 and let the compiler do that checking for us.
Flags: needinfo?(nfroyd)
(In reply to Nathan Froyd [:froydnj] from comment #18)

> Eh, if we had an equivalent of std::array::fill, we'd just have:
> 
>   ShearArray shear1;
>   shear1.fill(0.0f);
> 
> Worth noting that we'd have to make sure that the initializer_list was the
> length we expect at runtime, and we'd need a test or two.  Might be easier
> to adopt std::array's approach described in comment 16 and let the compiler
> do that checking for us.

The variadic constructor approach in the latest patch provides static checking of the length without making the inner array member public.
Flags: needinfo?(matt.woodrow)
(In reply to Matt Woodrow (:mattwoodrow) from comment #19)
> (In reply to Nathan Froyd [:froydnj] from comment #18)
> > Worth noting that we'd have to make sure that the initializer_list was the
> > length we expect at runtime, and we'd need a test or two.  Might be easier
> > to adopt std::array's approach described in comment 16 and let the compiler
> > do that checking for us.
> 
> The variadic constructor approach in the latest patch provides static
> checking of the length without making the inner array member public.

Oh my, I completely missed that; I think I had comment 16 all typed out on a previous version of the patch and then submitted it on the most recent version or something?  That's completely my fault and most of my comments are just noise, then.  The variadic constructor approach would be fine.

James, can you add tests for the new constructor, remove the InitializerList.h include, and resubmit for review, please?
Flags: needinfo?(jacheng)
Sure, I will create the test int mfbt/test...

Thank you
Flags: needinfo?(jacheng)
Attachment #8802408 - Flags: review?(nfroyd)
Hi Nathan,

I saw your patch runs OK on treeherder...

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=53252ffba01d8723024e875f3410153e0ffe3b1b

But I met ```fatal error: 'initializer_list' file not found```

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4d2b02b1dd0&selectedJob=29425809

I cannot find any magic code on your patch but I did not have any idea about the build error.

Could you please take a look and give me some hints?

Thanks.
Flags: needinfo?(nfroyd)
In fact, I don't need to include <initializer_list> anymore... So I delete this,  but I still wonder why I got the compile error...
(In reply to James Cheng[:JamesCheng] from comment #26)
> I saw your patch runs OK on treeherder...
> 
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&revision=53252ffba01d8723024e875f3410153e0ffe3b1b
> 
> But I met ```fatal error: 'initializer_list' file not found```
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=d4d2b02b1dd0&selectedJob=29425809
> 
> I cannot find any magic code on your patch but I did not have any idea about
> the build error.

Ah, so this is a little subtle.  OS X actually has two standard C++ libraries available: libstdc++ 4.2.1 or so (old, busted, but required for backwards compat) and libc++.  You can select which one you want to compile against with a command-line flag.  The clang that we compile with defaults to libstdc++.  We want to compile with the C++11 functionality of libc++, so we pass the appropriate command-line flag.  Except that we only do that for *target* code (basically, firefox, libxul, and everything called by those) and the error in your case comes from compiling *host* code (stuff that will run as part of the build process), so we're not passing the appropriate flag, and we default to using libstdc++.  I've filed bug 1311367 to fix this state of affairs.
Flags: needinfo?(nfroyd)
Comment on attachment 8800137 [details]
Bug 1309466 - Enable Array and EnumeratedArray to be brace-initialized with initial contents.

https://reviewboard.mozilla.org/r/85148/#review85938

This is OK, but the commit message should be amended to read "Enable Array and EnumeratedArray to be brace-initialized with initial contents" or somesuch, since we're no longer using `std::initializer_list`.
Attachment #8800137 - Flags: review?(nfroyd) → review+
Comment on attachment 8802408 [details]
Bug 1309466 - Add Test for the init value by constructor.

https://reviewboard.mozilla.org/r/86804/#review85940

Thank you for writing tests.  Comments below.

::: mfbt/tests/TestArray.cpp:21
(Diff revision 4)
> +  MOZ_RELEASE_ASSERT(arr1[2] == 3);
> +  // Style 2
> +  Array<int32_t, 3> arr2(5, 6);
> +  MOZ_RELEASE_ASSERT(arr2[0] == 5);
> +  MOZ_RELEASE_ASSERT(arr2[1] == 6);
> +  MOZ_RELEASE_ASSERT(arr2[2] == 0); // always need to be zero

I feel like this ability is going to cause pain and suffering, even if it's analogous to what C++ arrays would permit.  Can we `static_assert` that we get the correct number of args in the constructor (using `sizeof...(Args)` and if somebody really wants to pass fewer arguments, we can cross that bridge when we come to it?

::: mfbt/tests/TestArray.cpp:23
(Diff revision 4)
> +  Array<int32_t, 3> arr2(5, 6);
> +  MOZ_RELEASE_ASSERT(arr2[0] == 5);
> +  MOZ_RELEASE_ASSERT(arr2[1] == 6);
> +  MOZ_RELEASE_ASSERT(arr2[2] == 0); // always need to be zero
> +  // Style 3
> +  Array<int32_t, 3> arr3(7, 8, 9);

AFAICT, style 3 and style 1 are identical?  Did you mean to use braces in one or more of the styles, similar to how you did things for `EnumeratedArray`?
Attachment #8802408 - Flags: review?(nfroyd)
(In reply to Nathan Froyd [:froydnj] from comment #33)
> Comment on attachment 8802408 [details]
> Bug 1309466 - Add Test for the init value by constructor.
> 
> https://reviewboard.mozilla.org/r/86804/#review85940
> 
> Thank you for writing tests.  Comments below.
> 
> ::: mfbt/tests/TestArray.cpp:21
> (Diff revision 4)
> > +  MOZ_RELEASE_ASSERT(arr1[2] == 3);
> > +  // Style 2
> > +  Array<int32_t, 3> arr2(5, 6);
> > +  MOZ_RELEASE_ASSERT(arr2[0] == 5);
> > +  MOZ_RELEASE_ASSERT(arr2[1] == 6);
> > +  MOZ_RELEASE_ASSERT(arr2[2] == 0); // always need to be zero
> 
> I feel like this ability is going to cause pain and suffering, even if it's
> analogous to what C++ arrays would permit.  Can we `static_assert` that we
> get the correct number of args in the constructor (using `sizeof...(Args)`
> and if somebody really wants to pass fewer arguments, we can cross that
> bridge when we come to it?
> 
Thank you, I put the static_assert into the first patch.

Please take a look and correct me with the assert message I wrote (English).

> ::: mfbt/tests/TestArray.cpp:23
> (Diff revision 4)
> > +  Array<int32_t, 3> arr2(5, 6);
> > +  MOZ_RELEASE_ASSERT(arr2[0] == 5);
> > +  MOZ_RELEASE_ASSERT(arr2[1] == 6);
> > +  MOZ_RELEASE_ASSERT(arr2[2] == 0); // always need to be zero
> > +  // Style 3
> > +  Array<int32_t, 3> arr3(7, 8, 9);
> 
> AFAICT, style 3 and style 1 are identical?  Did you mean to use braces in
> one or more of the styles, similar to how you did things for
> `EnumeratedArray`?

I just want to demo and show the user that there are these three styles we can use to invoke the constructor to initialize the inner array.

If you think it is redundant I will remove it.

Thanks for your reminding and the answer for my build error which is very difficult to me to find the root cause.
(In reply to James Cheng[:JamesCheng] from comment #36)
> (In reply to Nathan Froyd [:froydnj] from comment #33)
> > AFAICT, style 3 and style 1 are identical?  Did you mean to use braces in
> > one or more of the styles, similar to how you did things for
> > `EnumeratedArray`?
> 
> I just want to demo and show the user that there are these three styles we
> can use to invoke the constructor to initialize the inner array.
> 
> If you think it is redundant I will remove it.

It is redundant, but I think you *meant* to do something different, which is why I am asking; compare the arrays in TestArray.cpp:

Array<int32_t, 3> arr1(1, 2, 3);
Array<int32_t, 3> arr2(5, 6);
Array<int32_t, 3> arr3(7, 8, 9);

Notice that they all use parentheses, especially |arr1| and |arr3|, which initialize the array in the exact same way.  Contrast TestEnumeratedArray.cpp:

EnumeratedArray<AnimalSpecies, AnimalSpecies::Count, int> headCount(1, 2, 3);
EnumeratedArray<AnimalSpecies, AnimalSpecies::Count, int> headCount2{5, 6, 7};
EnumeratedArray<AnimalSpecies, AnimalSpecies::Count, int> headCount3({8, 9, 10});

|headCount| here uses parentheses, |headCount2| uses braces, and |headCount3| uses both.  Did you intend for TestArray to use the same styles as TestEnumeratedArray or not?
Flags: needinfo?(jacheng)
Flags: needinfo?(mh+mozilla)
Sorry, I didn't notice that... It's the typo.

I wanna use the same style as TestEnumeratedArray.

I apologize for my careless. I will correct that and update for reviewing.

Thank you.
Flags: needinfo?(jacheng)
Comment on attachment 8802408 [details]
Bug 1309466 - Add Test for the init value by constructor.

Canceling this review, as there is a revised version coming according to comment 38.
Attachment #8802408 - Flags: review?(nfroyd)
Hi Nathan, I think I have updated the revised version already.

Could you please check it again?

Thank you!
Attachment #8802408 - Flags: review?(nfroyd)
Comment on attachment 8802408 [details]
Bug 1309466 - Add Test for the init value by constructor.

https://reviewboard.mozilla.org/r/86804/#review87626

My mistake!  I blame reviewboard for not sending out proper emails.
Attachment #8802408 - Flags: review?(nfroyd) → review+
Thank you for the review!
Keywords: checkin-needed
Please mark the resolve the pending issues in MozReview so this can autoland.
Done. Thanks for reminding.
Keywords: checkin-needed
Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/42d33a2ab63b
Enable Array and EnumeratedArray to be brace-initialized with initial contents. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/6ff1b39eeb61
Add Test for the init value by constructor. r=froydnj
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/42d33a2ab63b
https://hg.mozilla.org/mozilla-central/rev/6ff1b39eeb61
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Blocks: 1313554
You need to log in before you can comment on or make changes to this bug.