Remove AutoFallibleTArray

RESOLVED FIXED in Firefox 47

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: poiru, Assigned: poiru)

Tracking

(Blocks: 1 bug)

Trunk
mozilla47
Points:
---

Firefox Tracking Flags

(firefox46 affected, firefox47 fixed)

Details

Attachments

(7 attachments)

Comment hidden (empty)
(Assignee)

Comment 1

3 years ago
Created attachment 8702132 [details] [diff] [review]
Part 1: Rename nsAutoTArray to AutoTArray
Attachment #8702132 - Flags: review?(nfroyd)
(Assignee)

Comment 2

3 years ago
Created attachment 8702133 [details] [diff] [review]
Part 2: Switch some uses of AutoFallibleTArray to AutoTArray

This changes some function signatures to take a nsTArray<T>& instead of a
FallibleTArray<T>& because AutoTArray does not inherit from FallibleTArray.

This is effectively a no-op because the affected array operations already use
`mozilla::fallible`.
Attachment #8702133 - Flags: review?(nfroyd)
(Assignee)

Comment 3

3 years ago
Created attachment 8702134 [details] [diff] [review]
Part 3: Switch remaining uses of AutoFallibleTArray to AutoTArray

This is effectively a no-op because the affected array operations already use
`mozilla::fallible`.
Attachment #8702134 - Flags: review?(nfroyd)
(Assignee)

Comment 4

3 years ago
Created attachment 8702135 [details] [diff] [review]
Part 4: Remove AutoFallibleTArray
Attachment #8702135 - Flags: review?(nfroyd)
(Assignee)

Comment 5

3 years ago
Created attachment 8702136 [details] [diff] [review]
Part 5: Merge nsAutoArrayBase into AutoTArray

nsAutoArrayBase is no longer needed because AutoTArray is its only subclass.
Attachment #8702136 - Flags: review?(nfroyd)
(Assignee)

Comment 6

3 years ago
Created attachment 8702137 [details] [diff] [review]
Part 6: Rename AutoInfallibleTArray to AutoTArray

While we're here, lets get rid the useless AutoInfallibleTArray alias of AutoTArray.
Attachment #8702137 - Flags: review?(nfroyd)
(Assignee)

Comment 7

3 years ago
Created attachment 8702138 [details] [diff] [review]
Part 7: Remove AutoInfallibleTArray
Attachment #8702138 - Flags: review?(nfroyd)
Comment on attachment 8702132 [details] [diff] [review]
Part 1: Rename nsAutoTArray to AutoTArray

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

r=me assuming this was done with the moral equivalent of |perl -pi -e 's/nsAutoTArray/AutoTArray/g'|
Attachment #8702132 - Flags: review?(nfroyd) → review+
Attachment #8702133 - Flags: review?(nfroyd) → review+
Comment on attachment 8702134 [details] [diff] [review]
Part 3: Switch remaining uses of AutoFallibleTArray to AutoTArray

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

::: dom/bindings/BindingUtils.h
@@ +2087,5 @@
>  // Class for simple sequence arguments, only used internally by codegen.
>  namespace binding_detail {
>  
>  template<typename T>
> +class AutoSequence : public AutoTArray<T, 16>

This is the only bit that worries me, as keeping a fallible-methods-only class around makes life simpler for the code generator (fewer ways to screw up the code generator for infallible stuff--and for whatever uses mozilla::dom::Sequence).

Can you file a followup bug, cc bz and ehsan, and ni? bz about keeping something like that around?  Ehsan has said that he's not concerned about this, but bz might have a separate opinion as the bindings expert.
Attachment #8702134 - Flags: review?(nfroyd) → review+
Attachment #8702135 - Flags: review?(nfroyd) → review+
Attachment #8702136 - Flags: review?(nfroyd) → review+
Comment on attachment 8702137 [details] [diff] [review]
Part 6: Rename AutoInfallibleTArray to AutoTArray

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

r=me assuming this was done with the moral equivalent of |perl -pi -e 's/AutoInfallibleTArray/AutoTArray/g'|.
Attachment #8702137 - Flags: review?(nfroyd) → review+
Attachment #8702138 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd [:froydnj] from comment #9)
> Can you file a followup bug, cc bz and ehsan, and ni? bz about keeping
> something like that around?  Ehsan has said that he's not concerned about
> this, but bz might have a separate opinion as the bindings expert.

Or do the ni? here...either way, the landing of these patches should wait until that discussion is resolved.
(Assignee)

Comment 12

3 years ago
Right now, AutoSequence inherits from AutoFallibleTArray, which means that all allocating member functions must be used together with `mozilla:fallible`. If the `mozilla:fallible` parameter is not used, the compiler will emit an error.

This bug wants to remove AutoFallibleTArray and change AutoSequence to inherit from nsAutoTArray instead. This means that calls that already use `mozilla:fallible` will continue to work the same, but the compiler will no longer enforce the presence of `mozilla:fallible`. Calls that omit it will be infallible. Note that the few uses of AutoSequence in Codegen.py already use `mozilla::fallible` where appropriate.

bz, do you think this is an issue? (I can't ni? bz at the moment so just CCing him.)
So the situation is that all existing dom::binding_detail::AutoSequence consumers (not just in codegen but everywhere, though I kinda hope nothing outside codegen uses it) currently pass mozilla::fallible, right?  So we would only need to worry about new consumers, which should be carefully thinking about their stuff anyway?

If so, I'm probably OK with this.
(Assignee)

Comment 14

3 years ago
(In reply to Boris Zbarsky [:bz] (Vacation until Jan 4) from comment #13)
> So the situation is that all existing dom::binding_detail::AutoSequence
> consumers (not just in codegen but everywhere, though I kinda hope nothing
> outside codegen uses it) currently pass mozilla::fallible, right?  So we
> would only need to worry about new consumers, which should be carefully
> thinking about their stuff anyway?

Yes and yes.
You need to log in before you can comment on or make changes to this bug.