1487612 - [clang-tidy] Complain about a non-Append call immediate after call to SetCapacity() on nsA[C]String

Status: NEW

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Henri Sivonen (:hsivonen)]

XPCOM strings try to avoid heap-allocated buffers by pointing directly to POD literals, by pointing to adopted malloced buffers or by pointing to a heap-allocated buffer shared with another string.

XPCOM string also provide the method SetCapacity() for advising the string about an upcoming sequence of Append() calls so that multiple reallocations of the buffer can be avoided.

However, the heap allocation avoidance features easily undo the effects of SetCapacity() making SetCapacity() something of a footgun. SetCapacity() is often misused (called right before Assign(), Adopt(), Truncate(), etc.).

To that end, we should have a lint that complains if after
str.SetCapacity(n);
the next operation with str is not one of (or fallible variant of)
str.Append(...);
str.AppendASCII(...);
str.AppendLiteral(...); (once bug 1487606 has been fixed)
LossyAppendUTF16toASCII(foo, str);
AppendASCIItoUTF16(foo, str);
str.AppendPrintf(...);
str.AppendInt(...);
str.AppendFloat(...);

or there is only one such operation following str.SetCapacity(n);. (I.e. code looks correct only if str.SetCapacity(n); is followed by multiple operations from the above list.)

Comment 1

[(Away)]

Could we move this check up one stage in the development pipeline, i.e. discourage such code from being typed to begin with? If the append scenario is the only valid use for `SetCapacity`, perhaps it should have a name that's less inviting of other uses? I'd certainly think twice before using a `SetCapacityForMultipleAppends` for example.

(The clang-tidy rule could still be useful as an extra check)

Comment 2

[Henri Sivonen (:hsivonen)]

(In reply to David Major [:dmajor] from comment #1)
> Could we move this check up one stage in the development pipeline, i.e.
> discourage such code from being typed to begin with? If the append scenario
> is the only valid use for `SetCapacity`, perhaps it should have a name
> that's less inviting of other uses? I'd certainly think twice before using a
> `SetCapacityForMultipleAppends` for example.

Good point. Filed bug 1488142.

Comment 3

[(no longer active)]

CCing Andi in case he would like to take a stab at implementing this check, should be fairly simple, I think...

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

I should note that in some (many?) cases we won't be able to determine this statically; it will depend on <data> to decide how many (if any) Append calls get made, so (not surprisingly) this (especially the "one append" case, but others too) would not be definitive, but merely "look closely at this".  Sequences like SetCapacity(); Append(); Append(); will be obviously "ok" and SetCapacity(); Non-Append() will be clearly not-ok, but I suspect the majority will not be clearly ok or clearly not-ok (especially after a single pass through the existing code to catch the second)