[webkit-reviews] review canceled: [Bug 188328] Shrink size of PropertyCondition by packing UniquedStringImpl* and Kind : [Attachment 346580] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <utatane.tea at gmail.com> has canceled Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 188328: Shrink size of PropertyCondition by packing UniquedStringImpl* and
Kind
https://bugs.webkit.org/show_bug.cgi?id=188328

Attachment 346580: Patch

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




--- Comment #4 from Yusuke Suzuki <utatane.tea at gmail.com> ---
Comment on attachment 346580
  --> https://bugs.webkit.org/attachment.cgi?id=346580
Patch

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

>> Source/WTF/ChangeLog:7
>> +
> 
> You should explain the type you’re adding here

Added.

>> Source/WTF/wtf/PointerWithType.h:35
>> +template<typename PointerType, typename Type>
> 
> Do we want to static assert that Type is trivially constructable or maybe
even that it’s integral?

Sounds nice. I'll use `std::is_integral`.

>> Source/WTF/wtf/PointerWithType.h:36
>> +class PointerWithType {
> 
> The “WithType” feels like the wrong name here. It reads to me as if you’re
describing the type of the pointer. Maybe we can come up with a more
descriptive name, some ideas:
> - PointerTuple
> - CompactPointerTuple

CompactPointerTuple sounds good. Fixed.

>> Source/WTF/wtf/PointerWithType.h:38
>> +	static_assert(sizeof(Type) == 1, "");
> 
> Seems like you’d allow up to 2 bytes.

While JSValue typically only takes JSCells which are allocated from JS heap,
this type can take arbitrary pointer.
If our architecture starts using 5-level page tables, we may see 57bit
effective addresses in this type.
I would like to keep this size since I want to support x86_64 5-level page
table too where the effective pointer bits are 57bits.
I've reworked this patch to support it.

>> Source/WTF/wtf/PointerWithType.h:42
>> +#if USE(JSVALUE64)
> 
> I think more accurately you want this conditionalized on sizeof(void*) == 8
> 
> There are 64_32 architectures where pointers are 4 bytes

I've added a new `CPU(ADDRESS64)` and `CPU(ADDRESS32)` in Platform.h to switch
the implementation.

>> Source/WTF/wtf/PointerWithType.h:48
>> +	PointerType pointer() const { return bitwise_cast<PointerType>(m_data &
0x0000FFFFFFFFFFFFULL); }
> 
> This could be where things go wrong on 64_32 archs

OK, I've switched the implementation based on actual address width.