Vector::clear is a potential footgun

NEW
Unassigned

Status

()

Core
MFBT
a year ago
a year ago

People

(Reporter: jandem, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

a year ago
Vector::clear() sets the Vector's length to 0 but does not actually free the heap allocation (unlike Vector::clearAndFree()).

There are cases where not freeing makes sense, for instance for stack-allocated Vectors where we will free anyway when the Vector goes out of scope. However, I'm pretty sure many of the callers of clear() actually want clearAndFree() and this could reduce memory usage.

So I'd like to propose renaming clear() to clearNoFree(), clearWithoutFree(), or something.
nsTArray calls this operation TruncateLength; Rust calls this truncate.  Both are single-argument functions that require the provided argument to be a length that's less than the current length of the array, but differ on what happens when the provided length is greater than the current length.
std::vector::clear() and basically all other vector operations don't free() and this is somewhat well-known (e.g., Item 17 in Effective STL), so changing mozilla::Vector to different behavior might lead to the opposite footgun and a lot of unintended malloc()/free().  Even if all existing uses are converted to clearNoFree(), it probably won't stop people unaware of the change from making the mistake in the future.
In a "normal" project STL similarity might be a good selling point, but generally I think a lot of Mozillians are not particularly strong STL users, so assumptions about what "clear" means seem doubtful to be overall persuasive -- either way (to assist STL-familiar users, or to follow prior behavior).

What about clearAndFree/clearWithoutFreeing?  EIBTI, a little extra reading isn't going to hurt anyone, and then it's less mental overhead to remember the exact wrinkle of what "clear" means.
You need to log in before you can comment on or make changes to this bug.