[webkit-reviews] review granted: [Bug 234201] [WTF] Introduce TrailingArray : [Attachment 446924] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Dec 14 10:42:30 PST 2021


Darin Adler <darin at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 234201: [WTF] Introduce TrailingArray
https://bugs.webkit.org/show_bug.cgi?id=234201

Attachment 446924: Patch

https://bugs.webkit.org/attachment.cgi?id=446924&action=review




--- Comment #7 from Darin Adler <darin at apple.com> ---
Comment on attachment 446924
  --> https://bugs.webkit.org/attachment.cgi?id=446924
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=446924&action=review

I’m not thrilled by how much code is put in the class template definitions
here. Makes it a bit hard to see the overview of the class template.

> Source/WTF/wtf/EmbeddedFixedVector.h:45
> +	   return UniqueRef { *new (NotNull,
fastMalloc(Base::allocationSize(size))) EmbeddedFixedVector(size) };

Can we do without the type name entirely, and just use the braces?

> Source/WTF/wtf/EmbeddedFixedVector.h:51
> +	   unsigned size = std::distance(first, last);

What guarantees the distance can be shortened from size_t to unsigned? Do we
want a RELEASE_ASSERT of that? Or is there some way we know this is always
safe?

> Source/WTF/wtf/EmbeddedFixedVector.h:52
> +	   return UniqueRef { *new (NotNull,
fastMalloc(Base::allocationSize(size))) EmbeddedFixedVector(size, first, last)
};

Dito.

> Source/WTF/wtf/EmbeddedFixedVector.h:58
> +	   unsigned size = std::size(other);

Why call std::size instead of size() here?

Same question about why this fits in unsigned.

> Source/WTF/wtf/EmbeddedFixedVector.h:65
> +	   Vector<T, inlineCapacity, OverflowHandler> container =
WTFMove(other);

Why is it good to do this move? I’d think we’d want to move the *elements* out
of the vector and never move the vector itself, should just be able to use
std::move_iterator { other.begin() } below.

Also unclear on why we are using std::begin. It seems to me that we can just
use Vector::begin unless we have some reason we want to write code that’s more
generic.

Also, I would use auto here for the type if we were keeping this.

> Source/WTF/wtf/EmbeddedFixedVector.h:66
> +	   unsigned size = std::size(container);

Same question about narrowing to unsigned.

> Source/WTF/wtf/EmbeddedFixedVector.h:92
> +    EmbeddedFixedVector(unsigned size)

explicit

> Source/WTF/wtf/FixedVector.h:155
> +    Storage* getStorage() { return m_storage.get(); }

WebKit coding style says function should not be named with the word "get" in
it. We use get to mean "out arguments" or such.

> Source/WTF/wtf/TrailingArray.h:65
> +    TrailingArray(unsigned size, InputIterator first, InputIterator last)

Seems a little bit strange to take the size as an argument but also call
std::distance. I guess I’ll have to look at call sites to understand why.


More information about the webkit-reviews mailing list