Both AppendElements and SetLength default initialize the values, there's no
intermediate uninitialzed state. SetCapacity doesn't initialize the values,
but they're also not indexable -- we have release bounds assertions --
unless you try really hard.
nsTArray doesn't support emplace although it does have AppendElement(T&&),
but that wouldn't really help in this case. It's possible we could add that
of course!
-e
On Tue, Aug 1, 2017 at 1:11 PM, Kris Maglione <kmagli...@mozilla.com> wrote:
> On Tue, Aug 01, 2017 at 12:57:31PM -0700, Eric Rahm wrote:
>
>> nsTArray has various Append methods, in this case just using the
>> infallible
>> AppendElements w/o out a SetCapacity call would do the job. Another option
>> would be to use SetLength which would default initialize the new elements.
>>
>> If we're trying to make things fast-but-safeish in this case, the
>> preferred
>> way is probably:
>>
>> auto itr = AppendElements(length, fallible);
>> if (!itr) ...
>>
>> // no bounds checking
>> for (...; i++, itr++)
>> auto& mSocket = *itr;
>>
>> // bounds checking
>> for (...)
>> auto& mSocket = *sockets[i];
>>
>> In general I agree the pattern of fallibly allocating and then fallibly
>> appending w/o checking the return value is a bit silly. Perhaps we should
>> just mark the fallible version MUST_USE? It looks like it's commented out
>> for some reason (probably a ton of bustage).
>>
>
> Honestly, I think the Vector infallible* methods are a much cleaner way to
> handle this, especially when we need something like infallibleEmplaceBack.
> They tend to encourage writing more efficient code, with fewer
> reallocations and allocation checks. And I think the resulting code tends
> to be safer than AppendElements followed by unchecked raw pointer access,
> placement `new`s, and an intermediate state where we have a bunch of
> indexable but uninitialized elements in the array.
>
> That's been my experience when reading and writing code using the various
> approaches, anyway.
>
>
> On Tue, Aug 1, 2017 at 12:18 PM, Kris Maglione <kmagli...@mozilla.com>
>> wrote:
>>
>> On Tue, Aug 01, 2017 at 12:31:24PM -0400, Alexis Beingessner wrote:
>>>
>>> I was recently searching through our codebase to look at all the ways we
>>>> use fallible allocations, and was startled when I came across several
>>>> lines
>>>> that looked like this:
>>>>
>>>> dom::SocketElement &mSocket = *sockets.AppendElement(fallible);
>>>> ...
>>>> So in isolation this code is saying "I want to handle allocation
>>>> failure"
>>>> and then immediately not doing that and just dereferencing the result.
>>>> This
>>>> turns allocation failure into Undefined Behaviour, rather than a process
>>>> abort.
>>>>
>>>> Thankfully, all the cases where I found this were preceded by something
>>>> like the following:
>>>>
>>>> uint32_t length = socketData->mData.Length();if
>>>> (!sockets.SetCapacity(length, fallible)) { JS_ReportOutOfMemory(cx);
>>>> return NS_ERROR_OUT_OF_MEMORY;}for (uint32_t i = 0; i <
>>>> socketData->mData.Length(); i++) { dom::SocketElement &mSocket =
>>>> *sockets.AppendElement(fallible);
>>>>
>>>>
>>>>
>>>> So really, the fallible AppendElement *is* infallible, but we're just
>>>> saying "fallible" anyway. I find this pattern concerning for two
>>>> reasons:
>>>>
>>>>
>>> The MFBT Vector class handles this with a set of infallibleAppend[1]
>>> methods that assert that space has been reserved for the new elements
>>> before they're called. If we're using this pattern elsewhere without the
>>> same safeties, either those places should probably switch to using
>>> Vectors,
>>> or we should probably add similar checked infallible methods to nsTArray.
>>>
>>>
>>> [1]: http://searchfox.org/mozilla-central/rev/bbc1c59e460a27b2092
>>> 9b56489e2e55438de81fa/mfbt/Vector.h#707-733
>>>
>>> _______________________________________________
>>> dev-platform mailing list
>>> dev-platform@lists.mozilla.org
>>> https://lists.mozilla.org/listinfo/dev-platform
>>>
>>>
> --
> Kris Maglione
> Senior Firefox Add-ons Engineer
> Mozilla Corporation
>
> Science is interesting, and if you don't agree you can fuck off.
> --Nigel Calder, former editor of New Scientist
>
>
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
