Closed Bug 1332333 Opened 4 years ago Closed 4 years ago

rm EffectlesslyLookupProperty

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: jandem, Assigned: tcampbell)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 4 obsolete files)

This function used to have a lot  more code in it (for DOM proxies), but now it just forwards to LookupPropertyPure and we should just call LookupPropertyPure directly.
Assignee: nobody → tcampbell
Attachment #8830368 - Flags: review?(jdemooij)
Attachment #8830369 - Flags: review?(jdemooij)
Comment on attachment 8830368 [details] [diff] [review]
0001-Bug-1332333-Remove-uses-of-EffectlesslyLookupPropert.patch

Review of attachment 8830368 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! As mentioned below you have to rebase this though.

For these patches it doesn't matter, but most people post patches with 8 lines of context. I use Mercurial so I have these settings in my .hgrc file:

[diff]
git = 1
showfunc = true
unified = 8

::: js/src/jit/BaselineIC.cpp
@@ +2728,5 @@
>  
>      Rooted<PropertyResult> prop(cx);
>      RootedObject holder(cx);
> +    if (!LookupPropertyPure(cx, obj, id, holder.address(), prop.address()))
> +        return true;

Sorry, I landed a patch on mozilla-inbound earlier today that removes this code, so if you rebase you no longer need this change.
Attachment #8830368 - Flags: review?(jdemooij) → review+
Comment on attachment 8830369 [details] [diff] [review]
0002-Bug-1332333-Remove-EffectlessLookupProperty.-r-jande.patch

Review of attachment 8830369 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for removing the comments too!
Attachment #8830369 - Flags: review?(jdemooij) → review+
Rebased patch to m-i/tip. Forwarding r+.
Attachment #8830368 - Attachment is obsolete: true
Attachment #8830408 - Flags: review+
Attachment #8830408 - Attachment description: remove-effectlessly-lookup-property.patch → 0001-remove-effectlessly-lookup-property.patch
Attachment #8830408 - Attachment filename: remove-effectlessly-lookup-property.patch → 0001-remove-effectlessly-lookup-property.patch
Attached patch Remove EffectlessLookupProperty (obsolete) — Splinter Review
From c826868fe04b8784b79331d12ee6edb37ec166f3 Mon Sep 17 00:00:00 2001
Attachment #8830467 - Attachment is obsolete: true
Forwarding r+. Formatted patch and rebased on inbound/tip.
Attachment #8830408 - Attachment is obsolete: true
Attachment #8830547 - Flags: review+
Forward r+. Fix patch format.
Attachment #8830369 - Attachment is obsolete: true
Attachment #8830550 - Flags: review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7493fe7b336d
Remove uses of EffectlesslyLookupProperty. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bf2d6dea785
Remove EffectlessLookupProperty. r=jandem
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7493fe7b336d
https://hg.mozilla.org/mozilla-central/rev/8bf2d6dea785
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.