[webkit-reviews] review denied: [Bug 230283] [bmalloc] Simplify LargeRange constructors and remove meaningless one : [Attachment 438187] PATCH

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has denied Basuke Suzuki
<Basuke.Suzuki at sony.com>'s request for review:
Bug 230283: [bmalloc] Simplify LargeRange constructors and remove meaningless
one
https://bugs.webkit.org/show_bug.cgi?id=230283

Attachment 438187: PATCH

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




--- Comment #2 from Filip Pizlo <fpizlo at apple.com> ---
Comment on attachment 438187
  --> https://bugs.webkit.org/attachment.cgi?id=438187
PATCH

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

I get that auto is fun and cool, and for some kinds of code, it may be the
right thing to do.

But not in a malloc.  We spend lots more time reading malloc than writing it. 
So, auto is a bad trade-off.  It makes the code less self-documenting and it
takes away checking.  It makes the code less well documented because now I need
to check that largeRanges is really a collection of LargeRanges to know that
this is what you get from it and add to it, for example.  Using auto also takes
away checking; I like that the code before your change is explicitly stating
that it wants to work with LargeRanges, not just whatever types the collection
has.

So, I think that most of your change just makes the code harder to read and
less self-checked.  It's not a good trade-off for a malloc.

> Source/bmalloc/ChangeLog:19
> +	   Actually I still cannot understand the behavior or meaning of
physical page information stored in
> +	   LargeRange. This is the first step of the investigation to simplify
the case with full physical pages.

I strongly recommend you understand it deeply before making more changes.

> Source/bmalloc/bmalloc/Heap.cpp:68
> -	   m_largeFree.add(LargeRange(base,
Gigacage::size(gigacageKind(m_kind)), 0, 0, base));
> +	   m_largeFree.add({ base, Gigacage::size(gigacageKind(m_kind)), 0, 0,
base });

This change makes the code less clear.	I want to know that I'm creating
LargeRange.

> Source/bmalloc/bmalloc/Heap.cpp:159
> -    for (LargeRange& range : m_largeFree) {
> +    for (auto& range : m_largeFree) {

This change makes the code less clear.	I want to know that range is a
LargeRange.

> Source/bmalloc/bmalloc/Heap.cpp:247
> -    m_largeFree.add(LargeRange(chunk, size, startPhysicalSize,
totalPhysicalSize, physicalEnd));
> +    m_largeFree.add({ chunk, size, startPhysicalSize, totalPhysicalSize,
physicalEnd });

This change makes the code less clear.	I want to know that I'm creating
LargeRange.

> Source/bmalloc/bmalloc/Heap.cpp:481
> -	   std::pair<LargeRange, LargeRange> pair = range.split(prefixSize);
> +	   auto pair = range.split(prefixSize);

This change makes the code less clear.	I want to know that it's a pair of
LargeRanges.

> Source/bmalloc/bmalloc/Heap.cpp:487
> -	   std::pair<LargeRange, LargeRange> pair = range.split(size);
> +	   auto pair = range.split(size);

This change makes the code less clear.	I want to know that it's a pair of
LargeRanges.

> Source/bmalloc/bmalloc/Heap.cpp:542
> -    LargeRange range = m_largeFree.remove(alignment, size);
> +    auto range = m_largeFree.remove(alignment, size);

This change makes the code less clear.	I want to know that it's a LargeRange.

> Source/bmalloc/bmalloc/Heap.cpp:574
> -	   return LargeRange();
> +	   return { };

This change makes the code less clear.	I want to know that I'm returning a
LargeRange.

> Source/bmalloc/bmalloc/Heap.cpp:579
> -	   return LargeRange();
> +	   return { };

This change makes the code less clear.	I want to know that I'm returning a
LargeRange.

> Source/bmalloc/bmalloc/Heap.cpp:584
> -	   return LargeRange();
> +	   return { };

This change makes the code less clear.	I want to know that I'm returning a
LargeRange.

> Source/bmalloc/bmalloc/Heap.cpp:590
> -    return LargeRange(memory, size, size, size, static_cast<char*>(memory) +
size);
> +    return { memory, size };

This change makes the code less clear.	I want to know that I'm returning a
LargeRange.

> Source/bmalloc/bmalloc/Heap.cpp:612
> -    m_largeFree.add(LargeRange(object, size, size, size,
static_cast<char*>(object) + size));
> +    m_largeFree.add({ object, size });

This change makes the code less clear.	I want to know that I'm creating a
LargeRange.