[webkit-reviews] review granted: [Bug 211059] [JSC] CallData/ConstructData structs should include CallType/ConstructType : [Attachment 397642] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ross Kirsling
<ross.kirsling at sony.com>'s request for review:
Bug 211059: [JSC] CallData/ConstructData structs should include
CallType/ConstructType
https://bugs.webkit.org/show_bug.cgi?id=211059

Attachment 397642: Patch

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




--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 397642
  --> https://bugs.webkit.org/attachment.cgi?id=397642
Patch

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

> Source/JavaScriptCore/ChangeLog:12
> +	   getCallData/getConstructData return a CallType/ConstructType and
have a CallData/ConstructData out param,
> +	   and then *both* of these are passed side-by-side to
`call`/`construct`, which all seems a bit silly.
> +
> +	   This patch adds a CallType/ConstructType field to
CallData/ConstructData such that getCallData/getConstructData
> +	   can do exactly what their names suggest and such that
`call`/`construct` require one less overt parameter.

Thanks. Glad you considered my suggestion.

I will point out that "get" in WebKit coding style is supposed to be reserved
for functions with out arguments, so a name change is called for here. But I
don’t think "callData" and "constructData" are necessarily good names, because
they sound like verb phrases. So it might take a little work to think of better
names.

> Source/JavaScriptCore/runtime/CallData.h:43
>      Host,

Irritating inconsistency that we call it "Host" here and "native" in the union.

Might also want to move CallType inside the struct and name it CallData::Type.

> Source/JavaScriptCore/runtime/ConstructData.h:46
> -enum class ConstructType : unsigned {
> +enum class ConstructType : uint8_t {
>      None,
>      Host,
>      JS
>  };
>  
>  struct ConstructData {

Seems that CallData and ConstructData are not different. Maybe they were at
some point. At this point, I don’t understand why we have a separate type for
ConstructType and ConstructData.

> Source/JavaScriptCore/runtime/JSCJSValue.h:89
> -enum class CallType : unsigned;
> +enum class CallType : uint8_t;
>  struct CallData;
> -enum class ConstructType : unsigned;
> +enum class ConstructType : uint8_t;
>  struct ConstructData;

Seems unlikely that we need the forward declarations of CallType and
ConstructType here any more. Seems unlikely they are used by code that doesn’t
include the header that has the struct definitions in it.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:517
>      VM& vm, JSGlobalObject* globalObject, CallFrame* callFrame, JSString*
string, JSValue searchValue, CallData& callData,

Doesn’t it seem like this should be const CallData&?