Bug 209158 – AccessCase::canReplace should allow a Getter to replace an IntrinsicGetter

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Tadeu Zagallo]

...

[Tadeu Zagallo]

<rdar://problem/59222012>

[Tadeu Zagallo]

Created attachment 393707 [details]
Patch

[Keith Miller]

Comment on attachment 393707 [details]
Patch

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

> Source/JavaScriptCore/bytecode/AccessCase.cpp:681
> +        if (other.type() != type() && other.type() != IntrinsicGetter)

Isn't this just always false? I think you want an ||?

[Keith Miller]

Comment on attachment 393707 [details]
Patch

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

>> Source/JavaScriptCore/bytecode/AccessCase.cpp:681
>> +        if (other.type() != type() && other.type() != IntrinsicGetter)
> 
> Isn't this just always false? I think you want an ||?

Nvm, I just misread this as ==.

[Saam Barati]

Comment on attachment 393707 [details]
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:9
> +        When we override an intrinsic getter with an user defined getter, we might end up with the

"an user" => "a user"

> Source/JavaScriptCore/bytecode/AccessCase.cpp:680
> +    case Getter:

You're missing the other direction here, where |this| is InrinsicGetter, and the other is Getter

If you wanted to be fancy, you could have something like:

bool isInterchangeable(AccessType a, AccessType b)
{
    if (a == b)
        return true;
    if (a == Getter and b == IntrinsicGetter)
        return true;
    if (a == IntrinsicGettter and b == Getter)
      return true
}

and then just write this function in terms of that

[Tadeu Zagallo]

Comment on attachment 393707 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:9
>> +        When we override an intrinsic getter with an user defined getter, we might end up with the
> 
> "an user" => "a user"

I debated that, thanks.

>> Source/JavaScriptCore/bytecode/AccessCase.cpp:680
>> +    case Getter:
> 
> You're missing the other direction here, where |this| is InrinsicGetter, and the other is Getter
> 
> If you wanted to be fancy, you could have something like:
> 
> bool isInterchangeable(AccessType a, AccessType b)
> {
>     if (a == b)
>         return true;
>     if (a == Getter and b == IntrinsicGetter)
>         return true;
>     if (a == IntrinsicGettter and b == Getter)
>       return true
> }
> 
> and then just write this function in terms of that

I was unsure whether it was possible to go in the other direction. Fixed it now.

[Tadeu Zagallo]

Created attachment 393773 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 393773 [details]
Patch for landing

Clearing flags on attachment: 393773

Committed r258573: <https://trac.webkit.org/changeset/258573>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.