Closed
Bug 1463645
Opened 6 years ago
Closed 6 years ago
Unnecessary cast between JSObject* and JSFunction* in CreateFunctionPrototype
Categories
(Core :: JavaScript Engine, enhancement)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: arai, Assigned: maharsh312, Mentored)
Details
(Keywords: good-first-bug, Whiteboard: [lang=c++])
Attachments
(1 file, 2 obsolete files)
1.36 KB,
patch
|
arai
:
review+
|
Details | Diff | Splinter Review |
CreateFunctionPrototype calls NewFunctionWithProto and receives the return value as `JSObject* functionProto_`. https://searchfox.org/mozilla-central/rev/2aa42f2cab4a110edf21dd7281ac23a1ea8901f9/js/src/vm/JSFunction.cpp#838-841 > static JSObject* > CreateFunctionPrototype(JSContext* cx, JSProtoKey key) > { > ... > JSObject* functionProto_ = > NewFunctionWithProto(cx, nullptr, 0, JSFunction::INTERPRETED, > enclosingEnv, nullptr, objectProto, AllocKind::FUNCTION, > SingletonObject); NewFunctionWithProto returns JSFunction* https://searchfox.org/mozilla-central/rev/eb51d734a8c06ac5973291be9bcc74ed4e999bca/js/src/vm/JSFunction.h#813-817 > extern JSFunction* > NewFunctionWithProto(JSContext* cx, JSNative native, unsigned nargs, > JSFunction::Flags flags, HandleObject enclosingEnv, HandleAtom atom, > HandleObject proto, gc::AllocKind allocKind = gc::AllocKind::FUNCTION, > NewObjectKind newKind = GenericObject); where JSFunction is the derived class of JSObject. https://searchfox.org/mozilla-central/rev/eb51d734a8c06ac5973291be9bcc74ed4e999bca/js/src/vm/JSFunction.h#41 > class JSFunction : public js::NativeObject https://searchfox.org/mozilla-central/rev/eb51d734a8c06ac5973291be9bcc74ed4e999bca/js/src/vm/NativeObject.h#465 > class NativeObject : public ShapedObject https://searchfox.org/mozilla-central/rev/eb51d734a8c06ac5973291be9bcc74ed4e999bca/js/src/vm/ShapedObject.h#21 > class ShapedObject : public JSObject So, the assignment above performs upcast. and then it downcasts functionProto_ to JSFunction* with `&functionProto_->as<JSFunction>()` call. https://searchfox.org/mozilla-central/rev/2aa42f2cab4a110edf21dd7281ac23a1ea8901f9/js/src/vm/JSFunction.cpp#845 > static JSObject* > CreateFunctionPrototype(JSContext* cx, JSProtoKey key) > { > ... > RootedFunction functionProto(cx, &functionProto_->as<JSFunction>()); So, the upcast and downcast are completely unnecessary, and we can just receive the return value of NewFunctionWithProto into RootedFunction. You'll need basic knowledge of C++, and be able to build and test Firefox [1] or SpiderMonkey [2]. A skilled first-time contributor should be able to finish this in a month. Leave comments / questions here, or ask me (:arai) in IRC #introduction and #jsapi channels. [1] https://developer.mozilla.org/en-US/docs/Simple_Firefox_build [2] https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Build_Documentation
Assignee | ||
Comment 1•6 years ago
|
||
Hi, This is my first experience with open source community and I would like to work on this bug. Please guide me on how should I begin with and get the code base.
Reporter | ||
Comment 2•6 years ago
|
||
Hi, thank you for your comment :D please follow this document to setup the development environment (until the step just before running "./mach build"): https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build and then follow this document to build SpiderMonkey shell: https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Build_Documentation
Assignee | ||
Comment 3•6 years ago
|
||
I have found a remove upcast and downcast JSFunction.cpp. Now, which specific test file should I run to check my changes?
Reporter | ||
Comment 4•6 years ago
|
||
Great :) First, please confirm that you can build with your change applied. then, you need to run test which executes the code you've touched, and actually CreateFunctionPrototype is executed while starting the shell, so any test is fine (in jstests, jit-test), or just run JS shell and confirm that it doesn't crash.
Assignee | ||
Comment 5•6 years ago
|
||
As I changed JSObject to JSFunction in "JSFunction.cpp", I was able to remove the upcast. But, I am getting an error during the build if I remove "->as<JSFunction>()" which performs downcast. Can you help me with this? Do tell me if something is wrong with my understanding of bug.
Assignee | ||
Comment 6•6 years ago
|
||
Reporter | ||
Comment 7•6 years ago
|
||
Comment on attachment 8980536 [details] [diff] [review] Bug 1463645 - Removed unnecessary upcast and downcast of JSFunction Review of attachment 8980536 [details] [diff] [review]: ----------------------------------------------------------------- Thank you for your patch :D Please address the following comments, and then attach the updated patch, and set "review" flag to "?" and choose me as requestee next time! ::: js/src/vm/JSFunction.cpp @@ +835,4 @@ > * give it the guts to be one. > */ > RootedObject enclosingEnv(cx, &self->lexicalEnvironment()); > + please remove spaces here, and also please remove newline. @@ +836,5 @@ > */ > RootedObject enclosingEnv(cx, &self->lexicalEnvironment()); > + > + RootedFunction functionProto(cx, NewFunctionWithProto(cx, nullptr, 0, JSFunction::INTERPRETED, > + enclosingEnv, nullptr, objectProto, AllocKind::FUNCTION, please use spaces instead of tabs, and align "e" of "enclosing" with "c" of "cx" in the above line. @@ -838,5 @@ > - JSObject* functionProto_ = > - NewFunctionWithProto(cx, nullptr, 0, JSFunction::INTERPRETED, > - enclosingEnv, nullptr, objectProto, AllocKind::FUNCTION, > - SingletonObject); > - if (!functionProto_) please keep this null-check. this case can happen if Out Of Memory (OOM) happens inside NewFunctionWithProto, and this is to properly handle such case.
Attachment #8980536 -
Flags: feedback+
Reporter | ||
Updated•6 years ago
|
Assignee: nobody → maharsh312
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•6 years ago
|
||
Attachment #8980649 -
Flags: review?(arai.unmht)
Reporter | ||
Comment 9•6 years ago
|
||
Comment on attachment 8980649 [details] [diff] [review] Bug 1463645- Added a nullcheck and improved the indentation Review of attachment 8980649 [details] [diff] [review]: ----------------------------------------------------------------- Great Almost there! Please fold this and the previous patches into single one. ::: js/src/vm/JSFunction.cpp @@ +838,4 @@ > RootedFunction functionProto(cx, NewFunctionWithProto(cx, nullptr, 0, JSFunction::INTERPRETED, > + enclosingEnv, nullptr, objectProto, > + AllocKind::FUNCTION, > + SingletonObject)); this can be moved to previous line. we're using 99-chars per line for code and 80-chars per line for comment.
Attachment #8980649 -
Flags: review?(arai.unmht) → feedback+
Assignee | ||
Comment 10•6 years ago
|
||
Attachment #8980731 -
Flags: review?(arai.unmht)
Reporter | ||
Comment 11•6 years ago
|
||
Comment on attachment 8980731 [details] [diff] [review] Bug 1463645 - Removed unnecessary upcast and downcast of JSFunction (updated) Review of attachment 8980731 [details] [diff] [review]: ----------------------------------------------------------------- Excellent! Thank you for fixing this :D here's try run for automated testing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd6605bd336b3094bb2609f26766da9239fc3b2e once it completes without failure, you can add "checkin-needed" keyword to Tracking-Keywords field above, (you can set multiple keywords separated by ",") so that the patch is landed to mozilla-inbound which will be merged to mozilla-central shortly. also, you can mark previous patches "obsolete" in Details link in attachments, click "Details" next to attachment, click "edit details", check "obsolete", and "Submit". so that it's clear which patch is the latest one. for next time, when you submitting new attachment, you can mark them obsolete in the form, list under "Obsoletes"
Attachment #8980731 -
Flags: review?(arai.unmht) → review+
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•6 years ago
|
Attachment #8980536 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8980649 -
Attachment is obsolete: true
Comment 12•6 years ago
|
||
Pushed by apavel@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ab890e8f2582 Removed unnecessary upcast and downcast of JSFunction r=arai
Keywords: checkin-needed
Comment 13•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ab890e8f2582
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in
before you can comment on or make changes to this bug.
Description
•