Bug 200960 - Make it easier to pass pointers to WTFCrashWithInfo.
Summary: Make it easier to pass pointers to WTFCrashWithInfo.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-08-20 18:19 PDT by Mark Lam
Modified: 2019-08-20 18:38 PDT (History)
8 users (show)

See Also:


Attachments
proposed patch. (5.01 KB, patch)
2019-08-20 18:23 PDT, Mark Lam
saam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2019-08-20 18:19:34 PDT
Now, we don't have to explicitly cast them to uint64_ts first.  The template wrappers will take care of it for us.
Comment 1 Mark Lam 2019-08-20 18:23:12 PDT
Created attachment 376837 [details]
proposed patch.
Comment 2 Saam Barati 2019-08-20 18:26:58 PDT
Comment on attachment 376837 [details]
proposed patch.

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

> Source/WTF/wtf/Assertions.h:625
> +    static_assert(std::is_integral<T>::value || std::is_enum<T>::value || std::is_pointer<T>::value, "All types need to be bitwise_cast-able to integral type for logging");

seems like a bad name for a function that allows pointers.
Comment 3 Yusuke Suzuki 2019-08-20 18:27:43 PDT
Comment on attachment 376837 [details]
proposed patch.

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

r=me with comment too.

> Source/WTF/wtf/Assertions.h:553
> +ALWAYS_INLINE uint64_t wtfCrashArg(T* arg) { return reinterpret_cast<uint64_t>(arg); }

In ARM64_32, the pointer size is 32bit so we should cast it to 64bit by using static_cast.
So, I suggest using `static_cast<uintptr_t>(arg)` here.
Comment 4 Yusuke Suzuki 2019-08-20 18:29:39 PDT
Comment on attachment 376837 [details]
proposed patch.

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

>> Source/WTF/wtf/Assertions.h:553
>> +ALWAYS_INLINE uint64_t wtfCrashArg(T* arg) { return reinterpret_cast<uint64_t>(arg); }
> 
> In ARM64_32, the pointer size is 32bit so we should cast it to 64bit by using static_cast.
> So, I suggest using `static_cast<uintptr_t>(arg)` here.

Ah, no. reinterpret_cast<uintptr_t>(arg) is the way.
Comment 5 Mark Lam 2019-08-20 18:32:05 PDT
Comment on attachment 376837 [details]
proposed patch.

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

>>> Source/WTF/wtf/Assertions.h:553
>>> +ALWAYS_INLINE uint64_t wtfCrashArg(T* arg) { return reinterpret_cast<uint64_t>(arg); }
>> 
>> In ARM64_32, the pointer size is 32bit so we should cast it to 64bit by using static_cast.
>> So, I suggest using `static_cast<uintptr_t>(arg)` here.
> 
> Ah, no. reinterpret_cast<uintptr_t>(arg) is the way.

Fixed.

>> Source/WTF/wtf/Assertions.h:625
>> +    static_assert(std::is_integral<T>::value || std::is_enum<T>::value || std::is_pointer<T>::value, "All types need to be bitwise_cast-able to integral type for logging");
> 
> seems like a bad name for a function that allows pointers.

I'll rename it to isIntegralOrPointerType.
Comment 6 Mark Lam 2019-08-20 18:37:25 PDT
Thanks for the reviews.  Landed in r248930: <http://trac.webkit.org/r248930>.
Comment 7 Radar WebKit Bug Importer 2019-08-20 18:38:16 PDT
<rdar://problem/54538595>