[webkit-reviews] review denied: [Bug 124533] Ability to iterate over API::Array : [Attachment 217329] constification

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has denied Martin Hock <mhock at apple.com>'s
request for review:
Bug 124533: Ability to iterate over API::Array
https://bugs.webkit.org/show_bug.cgi?id=124533

Attachment 217329: constification
https://bugs.webkit.org/attachment.cgi?id=217329&action=review

------- Additional Comments from Daniel Bates <dbates at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=217329&action=review


Can we write a TestWebKitAPI test for this? The tests are in
<http://trac.webkit.org/browser/trunk/Tools/TestWebKitAPI/Tests/>.
Specifically, the WebKit2 tests are in
<http://trac.webkit.org/browser/trunk/Tools/TestWebKitAPI/Tests/WebKit2>. You
may find it beneficial to look at an existing test as an example. You can run
the tests by executing the script Tools/Scripts/run-test-webkit-api.

> Source/WebKit2/Shared/APIArray.h:62
> +	   Vector<RefPtr<Object>>::const_iterator it;

Private instance variables should be prefixed with "m_" per (3) of section
Names of the WebKit Code Style Guidelines,
<http://www.webkit.org/coding/coding-style.html#names-data-members>.

Although it isn't formerly written in our code style guidelines and members of
a class are private by default, I suggest explicitly adding a private label to
demarcate that these instance variables are private.

> Source/WebKit2/Shared/APIArray.h:63
> +	   Vector<RefPtr<Object>>::const_iterator end;

Private instance variables should be prefixed with "m_" per (3) of section
Names of the WebKit Code Style Guidelines,
<http://www.webkit.org/coding/coding-style.html#names-data-members>.

> Source/WebKit2/Shared/APIArray.h:73
> +	   void operator++()

This is OK as-is when this iterator is used in a C++11 range-based for-loop.
For your consideration, I suggest having this function return the iterator to
conform to the expected behavior of the prefix operator.

> Source/WebKit2/Shared/APIArray.h:88
> +	   inline bool operator==(const_iterator<T> &other) const { return it
== other.it; }

Nit: & should be on the left per (2) of section Pointer and References of the
WebKit Code Style Guidelines,
<http://www.webkit.org/coding/coding-style.html#pointers-cpp>.

> Source/WebKit2/Shared/APIArray.h:89
> +	   inline bool operator!=(const_iterator<T> &other) const { return it
!= other.it; }

Ditto.

> Source/WebKit2/Shared/APIArray.h:94
> +    const Vector<RefPtr<Object>>& m_elements;

Nit: This line should be indented. (*)

Although it isn't formerly written in our code style guidelines and members of
a class are private by default, I suggest explicitly adding a private label to
demarcate that these instance variables are private.

(*) We don't seem to explicitly document this in our code style guidelines.
Although, it's implied in various examples and is consistent with the style of
existing code in our repository. We should look to make this explicit in the
WebKit Code Style Guidelines as well as standardize whether to include or omit
the private label for class members.

> Source/WebKit2/Shared/APIArray.h:96
> +	   iterator_factory(Vector<RefPtr<Object>> &elements) :
m_elements(elements) { }

Nit: & should be on the left per (2) of section Pointer and References of the
WebKit Code Style Guidelines,
<http://www.webkit.org/coding/coding-style.html#pointers-cpp>.