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
