Closed Bug 1165731 Opened 8 years ago Closed 8 years ago

Remove unnecessary SetCapacity call in variant_storage_traits::storage_conversion

Categories

(Toolkit :: Storage, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: poiru, Assigned: poiru)

References

Details

Attachments

(1 file)

AppendElements already ensures that sufficient capacity is available so the explicit SetCapacity call is not needed. Removing it because code like this will cause -Wunused-result warnings after bug 968520 is done.
Comment on attachment 8606758 [details] [diff] [review]
Remove unnecessary SetCapacity call in variant_storage_traits::storage_conversion

I'll redirect to mak since I'm not familiar with this code really. However, IMO it seems really silly to explicitly use a FallibleTArray and the deliberately ignore the return value...
Attachment #8606758 - Flags: review?(bent.mozilla) → review?(mak77)
Blocks: 968520
Comment on attachment 8606758 [details] [diff] [review]
Remove unnecessary SetCapacity call in variant_storage_traits::storage_conversion

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

I think the idea behind setCapacity was an optimization for malloc... but i'd expect AppendElements to do that, since this is not a loop of single appends.
Indeed AppendElements uses EnsureCapacity, and the same is done by SetCapacity.

The fallibleTArray has been introduced in bug 610823 that introduces infallible arrays, but it was a fallible nsTArray before as well.
I think it's invoked in the variant constructor, so I'm not sure we can report any useful error from there. We could OOM crash, but for byte arrays that could also be very large, I don't think that'd be nice to the user. We could assert in debug builds, but that wouldn't prevent release errors.
Maybe we could store a status bool in the constructor, and check it in the getters? Thoughts?

Regardless, that's a bug that should be filed apart from this.
Attachment #8606758 - Flags: review?(mak77) → review+
I filed bug 1166204 for the fallibleTArray, please reply there.
https://hg.mozilla.org/mozilla-central/rev/5795f6ace557
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.