1433584 - nsTArray SetCapacity documentation is incorrect/misleading ("increase the capacity of this array object *by* the specified amount")

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Daniel Holbert [:dholbert]]

SetCapacity()'s documentation is incorrect/misleading. Right now it says:

  // This method may increase the capacity of this array object by the
  // specified amount. [...]
  // @param aCapacity The desired capacity of this array.
  [...]
  bool SetCapacity(size_type aCapacity, const mozilla::fallible_t&)

The first line sounds like the parameter is a *delta* (i.e. the desired  *increase* in capacity).

But then the @param line (as well as the ergonomics) imply that the parameter is the desired *total capacity*.

I think the former (delta) implication is incorrect, and the latter implication is correct.  We should fix the documentation to make that clearer.

Comment 1

[Daniel Holbert [:dholbert]]

This documentation problem dates back to the original nsTArray implementation in bug 316782.

The patch there had:


+    // Resize the storage if necessary to achieve the requested capacity.
+    // @param capacity     The requested number of array elements.
+    // @param elementSize  The size of an array element.
+    // @return False if insufficient memory is available; true otherwise.
+    PRBool EnsureCapacity(size_type capacity, size_type elementSize);
[...]
+    // This method may increase the capacity of this array object by the
+    // specified amount.  This method may be called in advance of several
+    // AppendElement operations to minimize heap re-allocations.  This method
+    // will not reduce the number of elements in this array.
+    // @param capacity  The desired capacity of this array.
+    void SetCapacity(size_type capacity) {
+      EnsureCapacity(capacity, sizeof(elem_type));
+    }


As you can see, SetCapacity was (and still is) just a wrapper for EnsureCapacity (which aims to "achieve the requested capacity" whose arg was "the requested number of array elements")

Given that, it's obvious that SetCapacity's "increase...by the specified amount" language was (& still is) incorrect.

Patch coming up, to address this surgically with s/by/to/ in the SetCapacity documentation.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: Bug 1433584: Fix misleading nsTArray::SetCapacity documentation to avoid implying that its arg is a delta.

DONTBUILD because comment-only

Review commit: https://reviewboard.mozilla.org/r/215994/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/215994/

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 8945930 [details]
Bug 1433584: Fix misleading nsTArray::SetCapacity documentation to avoid implying that its arg is a delta.

https://reviewboard.mozilla.org/r/215994/#review222020

Thanks!

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/45ef3089de4b
Fix misleading nsTArray::SetCapacity documentation to avoid implying that its arg is a delta. r=froydnj

Comment 5

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/45ef3089de4b