Bug 173622 – reinterpret_cast does not evaluate to constexpr

• Status: RESOLVED
• Resolution: FIXED
• Component: bmalloc

[Daewoong Jang]

MSVC does not evaluate reinterpret_cast as a constant expression.
This change is needed for supporting bmalloc on Windows(https://bugs.webkit.org/show_bug.cgi?id=143310).

[Daewoong Jang]

Created attachment 313445 [details]
patch

[Build Bot]

Attachment 313445 [details] did not pass style-queue:


ERROR: Source/bmalloc/bmalloc/Algorithm.h:55:  bitwise_cast is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daewoong Jang]

Created attachment 313554 [details]
patch

[Build Bot]

Attachment 313554 [details] did not pass style-queue:


ERROR: Source/bmalloc/bmalloc/Algorithm.h:56:  bitwise_cast is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

IIRC, std::memcpy is not constexpr in C++14. And reinterpret_cast is so too.
Thus, we cannot use this thing as constexpr.

I think we should prepare two versions of this function by overriding.
One for T*, and one for T.
And in T case, we do not perform reinterpret_cast<>. Instead, we will use static_cast<uintptr_t>(). In that case, we can make it as constexpr.
Because this roundUpToMultipleOf is sometimes used for size_t, this enforces compilers to calculate constant values for that case (with constant values).

And in the case of T*, we will just use reinterpret_cast<>. And we do not annotate constexpr for that.

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #5)
> IIRC, std::memcpy is not constexpr in C++14. And reinterpret_cast is so too.
> Thus, we cannot use this thing as constexpr.

We need to wait http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0202r0.html integration to the spec.

> 
> I think we should prepare two versions of this function by overriding.
> One for T*, and one for T.
> And in T case, we do not perform reinterpret_cast<>. Instead, we will use
> static_cast<uintptr_t>(). In that case, we can make it as constexpr.
> Because this roundUpToMultipleOf is sometimes used for size_t, this enforces
> compilers to calculate constant values for that case (with constant values).
> 
> And in the case of T*, we will just use reinterpret_cast<>. And we do not
> annotate constexpr for that.

[Daewoong Jang]

Created attachment 313610 [details]
patch

[Daewoong Jang]

(In reply to Yusuke Suzuki from comment #5)
> IIRC, std::memcpy is not constexpr in C++14. And reinterpret_cast is so too.
> Thus, we cannot use this thing as constexpr.
> 
> I think we should prepare two versions of this function by overriding.
> One for T*, and one for T.
> And in T case, we do not perform reinterpret_cast<>. Instead, we will use
> static_cast<uintptr_t>(). In that case, we can make it as constexpr.
> Because this roundUpToMultipleOf is sometimes used for size_t, this enforces
> compilers to calculate constant values for that case (with constant values).
> 
> And in the case of T*, we will just use reinterpret_cast<>. And we do not
> annotate constexpr for that.

I updated the patch as you suggested. Thanks!

[JF Bastien]

Comment on attachment 313610 [details]
patch

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

> We need to wait http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0202r0.html integration to the spec.

Unfortunately the committee is split on whether memcpy should be constexpr. I think http://wg21.link/p0476 may get you what you want... if the committee agrees to constexpr.

> Source/bmalloc/bmalloc/Algorithm.h:51
>  template<typename T> inline constexpr T mask(T value, uintptr_t mask)

Which types do we expect to get for T here? Are they all valid? Is widening or narrowing OK? I would use enable_if to check invariants are met.

> Source/bmalloc/bmalloc/Algorithm.h:71
>  template<typename T> inline T roundUpToMultipleOf(size_t divisor, T x)

Ditto on T.

[Geoffrey Garen]

Comment on attachment 313610 [details]
patch

What function requires mask() or roundUpToMultipleOf to be constexpr? Can we make them not constexpr?

[Daewoong Jang]

(In reply to Geoffrey Garen from comment #10)
> Comment on attachment 313610 [details]
> patch
> 
> What function requires mask() or roundUpToMultipleOf to be constexpr? Can we
> make them not constexpr?

It seems that constexpr at roundUpToMultipleOf() could be removed, but mask() needs to be constexpr due to Sizes::maskSizeClass(). And reinterpret_cast still couldn't be used as MSVC is complaining that "cannot convert from std::size_t to uintptr_t". If you think the constexpr of roundUpToMultipleOf() should be removed, I'll update the patch.

>> Source/bmalloc/bmalloc/Algorithm.h:51
>>  template<typename T> inline constexpr T mask(T value, uintptr_t mask)

>Which types do we expect to get for T here? Are they all valid? Is widening or >narrowing OK? I would use enable_if to check invariants are met.

>> Source/bmalloc/bmalloc/Algorithm.h:71
>>  template<typename T> inline T roundUpToMultipleOf(size_t divisor, T x)

>Ditto on T.

Regarding Bastien's review, could you let me know which types are valid and which are not? 

Thank you for review.

[Yusuke Suzuki]

Comment on attachment 313610 [details]
patch

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

>>> Source/bmalloc/bmalloc/Algorithm.h:51
>>>  template<typename T> inline constexpr T mask(T value, uintptr_t mask)
>> 
>> Which types do we expect to get for T here? Are they all valid? Is widening or narrowing OK? I would use enable_if to check invariants are met.
> 
> 

The original code using reinterpret_cast requires that it does not cause any widening and narrowing.
Let's add `static_assert(sizeof(T) == sizeof(uintptr_t), "");` in this function.

>>> Source/bmalloc/bmalloc/Algorithm.h:71
>>>  template<typename T> inline T roundUpToMultipleOf(size_t divisor, T x)
>> 
>> Ditto on T.
> 
> 

Ditto. Let's add `static_assert(sizeof(T) == sizeof(uintptr_t), "");`.

[Daewoong Jang]

Created attachment 314660 [details]
patch

[Daewoong Jang]

I think the patch is got a lot better. Thanks!

[Yusuke Suzuki]

Comment on attachment 314660 [details]
patch

r=me

[WebKit Commit Bot]

Comment on attachment 314660 [details]
patch

Clearing flags on attachment: 314660

Committed r219181: <http://trac.webkit.org/changeset/219181>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Filip Pizlo]

Comment on attachment 314660 [details]
patch

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

> Source/bmalloc/bmalloc/Algorithm.h:53
> +    static_assert(sizeof(T) == sizeof(uintptr_t), "sizeof(T) must be equal to sizeof(uintptr_t).");

What is the purpose of this assertion?

> Source/bmalloc/bmalloc/Algorithm.h:75
> +    static_assert(sizeof(T) == sizeof(uintptr_t), "sizeof(T) must be equal to sizeof(uintptr_t).");

What is the purpose of this assertion?

[Daewoong Jang]

(In reply to Filip Pizlo from comment #18)
> Comment on attachment 314660 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=314660&action=review
> 
> > Source/bmalloc/bmalloc/Algorithm.h:53
> > +    static_assert(sizeof(T) == sizeof(uintptr_t), "sizeof(T) must be equal to sizeof(uintptr_t).");
> 
> What is the purpose of this assertion?
> 
> > Source/bmalloc/bmalloc/Algorithm.h:75
> > +    static_assert(sizeof(T) == sizeof(uintptr_t), "sizeof(T) must be equal to sizeof(uintptr_t).");
> 
> What is the purpose of this assertion?

Please refer to comment #9 and #12.

[Filip Pizlo]

(In reply to Daewoong Jang from comment #19)
> (In reply to Filip Pizlo from comment #18)
> > Comment on attachment 314660 [details]
> > patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=314660&action=review
> > 
> > > Source/bmalloc/bmalloc/Algorithm.h:53
> > > +    static_assert(sizeof(T) == sizeof(uintptr_t), "sizeof(T) must be equal to sizeof(uintptr_t).");
> > 
> > What is the purpose of this assertion?
> > 
> > > Source/bmalloc/bmalloc/Algorithm.h:75
> > > +    static_assert(sizeof(T) == sizeof(uintptr_t), "sizeof(T) must be equal to sizeof(uintptr_t).");
> > 
> > What is the purpose of this assertion?
> 
> Please refer to comment #9 and #12.

Thanks. 

In the future, it would be useful to put information about this in the ChangeLog.