Bug 206816

Summary: Make getVTablePointer() an inline function to be compliant with WebKit style guidelines.
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, cmarcelo, commit-queue, darin, dbates, ews-watchlist, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=206804
Attachments:
Description Flags
proposed patch.
darin: review+
patch for landing.
none
patch for landing. none

Description Mark Lam 2020-01-26 21:19:01 PST
See https://bugs.webkit.org/show_bug.cgi?id=206804#c7.
Comment 1 Mark Lam 2020-01-26 21:22:37 PST
Created attachment 388823 [details]
proposed patch.
Comment 2 Darin Adler 2020-01-26 21:41:23 PST
Comment on attachment 388823 [details]
proposed patch.

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

> Source/WTF/wtf/PointerPreparations.h:36
>  #if __has_builtin(__builtin_get_vtable_pointer)

Why is this after the CPU(ARM64E) check. Wouldn't we want to use the builtin if it exists, regardless of what the CPU is?
Comment 3 Mark Lam 2020-01-26 21:43:19 PST
Comment on attachment 388823 [details]
proposed patch.

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

>> Source/WTF/wtf/PointerPreparations.h:36
>>  #if __has_builtin(__builtin_get_vtable_pointer)
> 
> Why is this after the CPU(ARM64E) check. Wouldn't we want to use the builtin if it exists, regardless of what the CPU is?

Good point.  I'll rearrange the #ifs.  Thanks.
Comment 4 Mark Lam 2020-01-26 21:48:58 PST
Created attachment 388824 [details]
patch for landing.
Comment 5 Darin Adler 2020-01-26 22:00:21 PST
Comment on attachment 388824 [details]
patch for landing.

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

> Source/WTF/wtf/PointerPreparations.h:34
>  #if __has_builtin(__builtin_get_vtable_pointer)

Oops, this isn’t good for non-clang compilers, like on Windows.
Comment 6 Darin Adler 2020-01-26 22:01:38 PST
Comment on attachment 388824 [details]
patch for landing.

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

>> Source/WTF/wtf/PointerPreparations.h:34
>>  #if __has_builtin(__builtin_get_vtable_pointer)
> 
> Oops, this isn’t good for non-clang compilers, like on Windows.

What Anders did for things like this in Compiler.h is:

    #if defined(__has_builtin) && __has_builtin(__builtin_get_vtable_pointer)

This apparently works on all the compilers.
Comment 7 Mark Lam 2020-01-26 22:06:13 PST
Created attachment 388826 [details]
patch for landing.
Comment 8 WebKit Commit Bot 2020-01-27 00:54:53 PST
Comment on attachment 388826 [details]
patch for landing.

Clearing flags on attachment: 388826

Committed r255139: <https://trac.webkit.org/changeset/255139>
Comment 9 WebKit Commit Bot 2020-01-27 00:54:55 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Radar WebKit Bug Importer 2020-01-27 00:55:18 PST
<rdar://problem/58911518>