WTF::Vector: Add comments to each member function.

WTF::Vector: Add comments to each member function.

This patch adds lots of comments to the public member functions of Vector.
Additionally, the order of the member functions is changed slightly, and
some inlined implementation becomes out-of-line, both for readability.

It still lacks class-level commentary, including the precise semantics of
iterator invalidation or the explanation of inline capacity. This will be
addressed later.

BUG=690816
Review-Url: https://codereview.chromium.org/2696123002
Cr-Commit-Position: refs/heads/master@{#450669}
Committed: https://chromium.googlesource.com/chromium/src/+/e6cfc1a979decf08475dcef3b095c8af9c8f2649

[Yuta Kitamura, 2017-02-15 06:51:10]

The CQ bit was checked by yutak@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 06:51:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yuta Kitamura, 2017-02-15 06:51:36]

yutak@chromium.org changed reviewers:
+ tzik@chromium.org

[Yuta Kitamura, 2017-02-15 06:51:37]

tzik: PTAL?
haraken: Just FYI.

[tzik, 2017-02-15 07:30:18]

lgtm

[haraken, 2017-02-15 07:31:40]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2017-02-15 07:31:41]

LGTM

https://codereview.chromium.org/2696123002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/wtf/Vector.h (right):

https://codereview.chromium.org/2696123002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/Vector.h:886: //

Add a comment and explain that we prefer using an iterator as much as possible
because at() and operator[] have an out-of-range CHECK.

https://codereview.chromium.org/2696123002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/Vector.h:887: // TODO(yutak): Why do we need both?

If |vector| is a pointer, we want to write like:

  vector->at(120);

instead of:

  (*vector)[120];

https://codereview.chromium.org/2696123002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/Vector.h:901: // invalidated on a reallocation.

// You should not hold a returned pointer because the buffer address may change
when it's reallocated or moved (by Oilpan's heap compaction).

https://codereview.chromium.org/2696123002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/Vector.h:964: void reserveInitialCapacity(size_t
initialCapacity);

Is this really worth having? Can we just use reserveCapacity()?

[commit-bot: I haz the power, 2017-02-15 08:33:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-15 08:33:43]

Dry run: This issue passed the CQ dry run.

[Yuta Kitamura, 2017-02-15 10:01:39]

I just fixed the documentation of at(). See below for
the others.

https://codereview.chromium.org/2696123002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/wtf/Vector.h (right):

https://codereview.chromium.org/2696123002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/Vector.h:886: //
On 2017/02/15 07:31:41, haraken wrote:
> 
> Add a comment and explain that we prefer using an iterator as much as possible
> because at() and operator[] have an out-of-range CHECK.

This does not really make sense. I'm rather opposed to have
RELEASE_ASSERT here; if we need to protect such access, every access
must be protected, including accesses via iterators. If this assertion
forces us a strange coding style, something is very wrong.

Elliott's principle includes "fast". This assertion directly contradicts
it.

https://codereview.chromium.org/2696123002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/Vector.h:887: // TODO(yutak): Why do we need both?
On 2017/02/15 07:31:41, haraken wrote:
> 
> If |vector| is a pointer, we want to write like:
> 
>   vector->at(120);
> 
> instead of:
> 
>   (*vector)[120];

Hmm okay. This does not seem like a clear win, though.

https://codereview.chromium.org/2696123002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/Vector.h:901: // invalidated on a reallocation.
On 2017/02/15 07:31:41, haraken wrote:
> 
> // You should not hold a returned pointer because the buffer address may
change
> when it's reallocated or moved (by Oilpan's heap compaction).
> 

I'll mention those aspects in the class-level documentation (which
I'll do next).

https://codereview.chromium.org/2696123002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/Vector.h:964: void reserveInitialCapacity(size_t
initialCapacity);
On 2017/02/15 07:31:41, haraken wrote:
> 
> Is this really worth having? Can we just use reserveCapacity()?
> 

The implementation is much simpler, and there are many use sites.

[Yuta Kitamura, 2017-02-15 10:01:57]

The CQ bit was checked by yutak@chromium.org

[Yuta Kitamura, 2017-02-15 10:01:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, tzik@chromium.org
Link to the patchset: https://codereview.chromium.org/2696123002/#ps20001
(title: "Mention the use case of at().")

[commit-bot: I haz the power, 2017-02-15 10:02:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-15 11:40:49]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1487152917604600,
"parent_rev": "fab934e8345d0d0217811ae184129bb43a33ba8b", "commit_rev":
"e6cfc1a979decf08475dcef3b095c8af9c8f2649"}

[commit-bot: I haz the power, 2017-02-15 11:41:28]

Description was changed from

==========
WTF::Vector: Add comments to each member function.

This patch adds lots of comments to the public member functions of Vector.
Additionally, the order of the member functions is changed slightly, and
some inlined implementation becomes out-of-line, both for readability.

It still lacks class-level commentary, including the precise semantics of
iterator invalidation or the explanation of inline capacity. This will be
addressed later.

BUG=690816
==========

to

==========
WTF::Vector: Add comments to each member function.

This patch adds lots of comments to the public member functions of Vector.
Additionally, the order of the member functions is changed slightly, and
some inlined implementation becomes out-of-line, both for readability.

It still lacks class-level commentary, including the precise semantics of
iterator invalidation or the explanation of inline capacity. This will be
addressed later.

BUG=690816

Review-Url: https://codereview.chromium.org/2696123002
Cr-Commit-Position: refs/heads/master@{#450669}
Committed:
https://chromium.googlesource.com/chromium/src/+/e6cfc1a979decf08475dcef3b095...
==========

[commit-bot: I haz the power, 2017-02-15 11:41:29]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/e6cfc1a979decf08475dcef3b095...