Closed
Bug 1447442
Opened 6 years ago
Closed 6 years ago
Remove unused functions and clean-up various calls
Categories
(Core :: JavaScript Engine, enhancement, P3)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: anba, Assigned: anba)
Details
Attachments
(16 files, 1 obsolete file)
9.98 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
8.26 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
2.81 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
9.29 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
32.97 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
35.97 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
10.86 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
7.79 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
1.41 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
5.40 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
2.72 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
2.77 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
9.02 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
14.67 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
13.27 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
5.99 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
FullParseHandler::setComprehensionLambdaBody(...) is no longer used and can be removed.
Updated•6 years ago
|
Priority: -- → P3
Assignee | ||
Comment 1•6 years ago
|
||
Morph this bug into a more general clean-up issue.
Summary: Remove FullParseHandler::setComprehensionLambdaBody → Remove unused functions and clean-up various calls
Assignee | ||
Comment 2•6 years ago
|
||
- Remove some unused functions and variables.
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Attachment #8965514 -
Flags: review?(jorendorff)
Assignee | ||
Comment 3•6 years ago
|
||
- Directly call NAME_OFFSET instead of using the alias EAGER_ATOM. - Use WellKnownSymbols::get(JS::SymbolCode) instead of manually casting a JS::SymbolCode to an uint32_t/size_t. - Merge AsyncFromSyncIteratorObject::set{Iterator, NextMethod}() into a single AsyncFromSyncIteratorObject::init() method similar to other init() methods in this file. - Use AsTaggedProto(nullptr) instead of rooting TaggedProto(nullptr). - Use GlobalObject::getOrCreateConstructor() instead of GlobalObject::ensureConstructor() + GlobalObject::getConstructor().
Attachment #8965515 -
Flags: review?(jorendorff)
Assignee | ||
Comment 4•6 years ago
|
||
- Remove the (basically) unused ValueArray and JSValueArray classes. - (Passing raw |Value*| in ComputeThis seems a bit questionable...)
Attachment #8965516 -
Flags: review?(jorendorff)
Assignee | ||
Comment 5•6 years ago
|
||
- Move declarations of IntlClass, JSONClass, and MathClass out of JSObject.h into more reasonable headers. - Remove unused jsmath.h include in vm/SavedStacks.h (noticed while removing unnecessary includes in vm/GlobalObject.cpp; the removal was also verified by IWYU). - Make GlobalObject::classIsInitialized() and GlobalObject::functionObjectClassesInitialized() private methods similar to the other xyzIsInitialized methods in GlobalObject. - js_[gs]etter_str are only used in vm/JSObject.cpp, so move them into that file.
Attachment #8965517 -
Flags: review?(jorendorff)
Assignee | ||
Comment 6•6 years ago
|
||
Remove various variants of: RootedObject obj(cx, NewObjectWithGivenProto(cx, &SpecificObject::class_, proto)); if (!obj) return nullptr; Handle<SpecificObject*> specificObj = obj.as<SpecificObject>(); with the more direct: Rooted<SpecificObject*> obj(cx, NewObjectWithGivenProto<SpecificObject>(cx, proto)); if (!obj) return nullptr; - For this to work, a new NewObjectWithGivenTaggedProto version was necessary and a default parameter was necessary for one NewObjectWithGivenProto function. - Also remove extra rooting when the object is directly returned resp. assigned to another rooted variable. - Remove the template parameter of OrdinaryCreateFromConstructor because this function is only used for AsyncGenerator objects and because it makes it easier to format the code correctly. - Directly call NewObjectWithGivenProto instead of NewObjectWithClassProto in TypedArrayObjectTemplate::makeProtoInstance() because the |proto| parameter is always non-nullptr, which means the class-proto fallback in NewObjectWithClassProto is never used.
Attachment #8965518 -
Flags: review?(jorendorff)
Assignee | ||
Comment 7•6 years ago
|
||
- Instead of passing HandleObject to the various InitThingClass functions and then casting that HandleObject to GlobalObject, we can directly pass Handle<GlobalObject*> to the init-functions. - Also moved the declaration of InitProxyClass from js/public/Proxy.h to js/src/proxy/Proxy.h, so it matches the location of its definition in js/src/proxy/Proxy.cpp and removed the no longer needed JS_FRIEND_API annotation. - Some of the init-functions asserted that the passed global is a native object. But now that we directly pass Handle<GlobalObject*>, this assertion doesn't seem too useful to me anymore.
Attachment #8965519 -
Flags: review?(jorendorff)
Comment 8•6 years ago
|
||
Comment on attachment 8965514 [details] [diff] [review] bug1447442-part1-remove-unused.patch Review of attachment 8965514 [details] [diff] [review]: ----------------------------------------------------------------- Yes please.
Attachment #8965514 -
Flags: review?(jorendorff) → review+
Updated•6 years ago
|
Attachment #8965515 -
Flags: review?(jorendorff) → review+
Comment 9•6 years ago
|
||
Comment on attachment 8965516 [details] [diff] [review] bug1447442-part3-remove-valuearray.patch Review of attachment 8965516 [details] [diff] [review]: ----------------------------------------------------------------- Some days my job is so easy
Attachment #8965516 -
Flags: review?(jorendorff) → review+
Updated•6 years ago
|
Attachment #8965517 -
Flags: review?(jorendorff) → review+
Assignee | ||
Comment 10•6 years ago
|
||
Let's add some more clean-ups! - Move the TypedArrayLength enum to the MacroAssembler, because it's only used there. - Add a helper function to compute the |Scalar::Type| from a |Class*|, so we don't need to copy the code in three different places. Well, they weren't exact copies, because every place used a different style to cast. :-) - Apply the changes mentioned in this comment <https://searchfox.org/mozilla-central/rev/7ccb618f45a1398e31a086a009f87c8fd3a790b6/js/src/vm/TypedArrayObject.cpp#1408-1420>. Except that TypedArray_lengthGetter wasn't removed, because it's also used in |TypedArrayObject::isOriginalLengthGetter(...)|. - Remove forward declaration of JSProperty struct, because JSProperty is no longer present.
Attachment #8966271 -
Flags: review?(jorendorff)
Assignee | ||
Comment 11•6 years ago
|
||
Prefer UniqueChars/UniqueTwoByteChars over UniquePtr<char[], JS::FreePolicy> resp. UniquePtr<char16_t[], JS::FreePolicy>.
Attachment #8966273 -
Flags: review?(jorendorff)
Assignee | ||
Comment 12•6 years ago
|
||
These four JSWhyMagic constants seem to be unused now.
Attachment #8966274 -
Flags: review?(jorendorff)
Assignee | ||
Comment 13•6 years ago
|
||
js/public/HashTable.h - We now require GCC6 (bug 1444274), so we can remove the GCC < 6 workaround. js/src/jslibmath.h - std::copysign is available since c++11, so we don't need the #ifdefs anymore. - And we only need to include "js/Value.h" instead of "jsnum.h" here. js/src/jsmath.{h,cpp} - Move include for <cmath> over to jsmath.cpp. js/src/jsnum.h - Since we require GCC >= 6.1, we don't need to test for GCC >= 5. - But then we need to test that __clang__ is not defined, because Clang (may?) also define __GNUC__.
Attachment #8966275 -
Flags: review?(jorendorff)
Comment 14•6 years ago
|
||
Comment on attachment 8966275 [details] [diff] [review] bug1447442-part10-old-compiler-ifdefs.patch Review of attachment 8966275 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsnum.h @@ +18,5 @@ > > > // This macro is should be `one' if current compiler supports builtin functions > // like __builtin_sadd_overflow. > +#if __GNUC__ && !defined(__clang__) MOZ_IS_GCC will work for this, with "mozilla/Compiler.h".
Assignee | ||
Comment 15•6 years ago
|
||
(In reply to Jeff Walden [:Waldo] from comment #14) > MOZ_IS_GCC will work for this, with "mozilla/Compiler.h". Thanks! :-)
Assignee | ||
Comment 16•6 years ago
|
||
Updated to use MOZ_IS_GCC per comment #14.
Attachment #8966275 -
Attachment is obsolete: true
Attachment #8966275 -
Flags: review?(jorendorff)
Attachment #8966350 -
Flags: review?(jorendorff)
Assignee | ||
Comment 17•6 years ago
|
||
Use MOZ_IS_GCC in two more files.
Attachment #8966351 -
Flags: review?(jwalden+bmo)
Comment 18•6 years ago
|
||
Comment on attachment 8965518 [details] [diff] [review] bug1447442-part5-templated-create.patch Review of attachment 8965518 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/ModuleObject.cpp @@ +148,5 @@ > RootedObject proto(cx, GlobalObject::getOrCreateImportEntryPrototype(cx, cx->global())); > if (!proto) > return nullptr; > > + ImportEntryObject* self = NewObjectWithGivenProto<ImportEntryObject>(cx, proto); Since the immediately preceding code grabs the standard prototype object, shouldn't we use NewObjectWithClassProto for stuff like this? There seem to be a lot of places that should change, maybe a majority of the call sites.
Attachment #8965518 -
Flags: review?(jorendorff) → review+
Assignee | ||
Comment 19•6 years ago
|
||
(In reply to Jason Orendorff [:jorendorff] from comment #18) > Since the immediately preceding code grabs the standard prototype object, > shouldn't we use NewObjectWithClassProto for stuff like this? There seem to > be a lot of places that should change, maybe a majority of the call sites. ImportEntryObject::class_ doesn't have a ProtoKey, so we can't derive the correct prototype from the |Class*| in [1] and therefore we need to use NewObjectWithGivenProto here. We could probably use NewObjectWithClassProto in |js::ErrorObject::create|. And the NewObjectWithGivenProto callers which always pass |proto == nullptr| could be changed to use NewObjectWithNullTaggedProto instead. [1] https://searchfox.org/mozilla-central/rev/b55e1a1cbcaee34878e133fbac20c4c2af6e11b5/js/src/vm/JSObject.cpp#835-843
Updated•6 years ago
|
Attachment #8966351 -
Flags: review?(jwalden+bmo) → review+
Comment 20•6 years ago
|
||
Comment on attachment 8965519 [details] [diff] [review] bug1447442-part6-init-global.patch Review of attachment 8965519 [details] [diff] [review]: ----------------------------------------------------------------- Great.
Attachment #8965519 -
Flags: review?(jorendorff) → review+
Updated•6 years ago
|
Attachment #8966271 -
Flags: review?(jorendorff) → review+
Updated•6 years ago
|
Attachment #8966273 -
Flags: review?(jorendorff) → review+
Updated•6 years ago
|
Attachment #8966274 -
Flags: review?(jorendorff) → review+
Updated•6 years ago
|
Attachment #8966350 -
Flags: review?(jorendorff) → review+
Assignee | ||
Comment 21•6 years ago
|
||
Removes the unused HAVE_HYPOT definition from old-configure.in and updates some comments for Math.hypot.
Attachment #8967542 -
Flags: review?(jorendorff)
Assignee | ||
Comment 22•6 years ago
|
||
More unused declarations which can be removed.
Attachment #8967543 -
Flags: review?(jorendorff)
Assignee | ||
Comment 23•6 years ago
|
||
Removes unused parameters for - GlobalObject::createBlankPrototypeInheriting / CreateBlankProto - and js::InitClass / DefineConstructorAndPrototype (All callers of js::InitClass use the default parameter |ctorKind == AllocKind::FUNCTION, which also happens to be default parameter value for NewNativeConstructor <https://searchfox.org/mozilla-central/rev/9f3da81290054c5b8955bb67ff98cae66676f745/js/src/vm/JSFunction.h#820>, so we can the AllocKind parameter js::InitClass / DefineConstructorAndPrototype.)
Attachment #8967551 -
Flags: review?(jorendorff)
Assignee | ||
Comment 24•6 years ago
|
||
Potpourri of other small changes where it didn't make sense to split them in separate patches: js/src/frontend/FoldConstants.cpp - Directly call NumberMod resp. NumberDiv instead of copying the exact same functions. js/src/jsfriendapi.h, js/src/proxy/Proxy.cpp - Remove the declaration from jsfriendapi. js/src/vm/Debugger-inl.h, js/src/vm/Debugger.cpp, js/src/vm/PIC.h - Directly call JSClass::getClass() instead of going through jsfriendapi when we're already in internal classes which can see JSClass::getClass(). js/src/vm/Debugger.{h,cpp} - Change the initClass() functions to take a Handle<GlobalObject*> and change the slightly confusing parameter names like |objProto| when referring to the global object. :-)
Attachment #8967554 -
Flags: review?(jorendorff)
Assignee | ||
Comment 25•6 years ago
|
||
Clean-up jspubtd.h to remove unnecessary #includes and forward declarations. Also replace |MOZ_BEGIN_EXTERN_C| with |extern "C"| as suggested in <https://searchfox.org/mozilla-central/rev/9f3da81290054c5b8955bb67ff98cae66676f745/mfbt/Types.h#120-121>. The additional includes for the non-SpiderMonkey files were added to fix the builds: https://treeherder.mozilla.org/#/jobs?repo=try&revision=63a2697197f8668a854b89d3df4b6ce0128dfe53 https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a6035ca635ad9d46d2a31d5d298920f412a7f5f Looks like someone forgot to add some #includes. ;-)
Attachment #8967557 -
Flags: review?(jorendorff)
Updated•6 years ago
|
Attachment #8967542 -
Flags: review?(jorendorff) → review+
Updated•6 years ago
|
Attachment #8967543 -
Flags: review?(jorendorff) → review+
Comment 26•6 years ago
|
||
Comment on attachment 8967551 [details] [diff] [review] bug1447442-part14-unused-parameters.patch Review of attachment 8967551 [details] [diff] [review]: ----------------------------------------------------------------- In the commit message: >so we can the AllocKind parameter js::InitClass / DefineConstructorAndPrototype This sentence no verb.
Attachment #8967551 -
Flags: review?(jorendorff) → review+
Updated•6 years ago
|
Attachment #8967554 -
Flags: review?(jorendorff) → review+
Updated•6 years ago
|
Attachment #8967557 -
Flags: review?(jorendorff) → review+
Assignee | ||
Comment 27•6 years ago
|
||
(In reply to Jason Orendorff [:jorendorff] from comment #26) > In the commit message: > Thankfully only in the bugzilla comment and not in the commit message. :-)
Assignee | ||
Comment 28•6 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7fe435408a5724fd2a1c4ff7a3163686f9724a30
Keywords: checkin-needed
Comment 29•6 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/fde411a83e7f Part 1: Remove unused functions and definitions. r=jorendorff https://hg.mozilla.org/integration/mozilla-inbound/rev/f19218382734 Part 2: Clean-up various calls to clarity. r=jorendorff https://hg.mozilla.org/integration/mozilla-inbound/rev/236f58368a50 Part 3: Remove JSValueArray and ValueArray. r=jorendorff https://hg.mozilla.org/integration/mozilla-inbound/rev/596553645783 Part 4: Move some definitions to where they are used. r=jorendorff https://hg.mozilla.org/integration/mozilla-inbound/rev/a5cdb5568e44 Part 5: Use template versions of NewBuiltinClassInstance and NewObjectWithClassProto instead of manual casting. r=jorendorff https://hg.mozilla.org/integration/mozilla-inbound/rev/7a03a7e15320 Part 6: Pass Handle<GlobalObject*> in ClassInitializerOp. r=jorendorff https://hg.mozilla.org/integration/mozilla-inbound/rev/20546b86221e Part 7: More clean-up for typed arrays and array buffer views. r=jorendorff https://hg.mozilla.org/integration/mozilla-inbound/rev/6eae71b641b2 Part 8: Use UniqueChars and UniqueTwoByteChars typedefs in a few places. r=jorendorff https://hg.mozilla.org/integration/mozilla-inbound/rev/0d6754739bf9 Part 9: Remove unused JSWhyMagic constants. r=jorendorff https://hg.mozilla.org/integration/mozilla-inbound/rev/8bae4384ab4c Part 10: Remove ifdefs for unsupported compiler versions. r=jorendorff https://hg.mozilla.org/integration/mozilla-inbound/rev/109933956ea9 Part 11: Use MOZ_IS_GCC to test for GCC. r=Waldo https://hg.mozilla.org/integration/mozilla-inbound/rev/2b300e277494 Part 12: Remove unused HAVE_HYPOT from old-configure and update comments in hypot function. r=jorendorff https://hg.mozilla.org/integration/mozilla-inbound/rev/70d1fd2ca5c6 Part 13: Remove additional unused declarations. r=jorendorff https://hg.mozilla.org/integration/mozilla-inbound/rev/bedd95bcb798 Part 14: Remove unused parameters from GlobalObject::createBlankPrototypeInheriting and js::InitClass. r=jorendorff https://hg.mozilla.org/integration/mozilla-inbound/rev/206bd2d6cda4 Part 15: Potpourri of changes to use more concrete types, move a declaration, avoid code duplication. r=jorendorff https://hg.mozilla.org/integration/mozilla-inbound/rev/75f301551f98 Part 16: Replace MOZ_BEGIN_EXTERN_C in C++ header jspubtd.h and remove unused includes and declarations. r=jorendorff
Keywords: checkin-needed
Comment 30•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fde411a83e7f https://hg.mozilla.org/mozilla-central/rev/f19218382734 https://hg.mozilla.org/mozilla-central/rev/236f58368a50 https://hg.mozilla.org/mozilla-central/rev/596553645783 https://hg.mozilla.org/mozilla-central/rev/a5cdb5568e44 https://hg.mozilla.org/mozilla-central/rev/7a03a7e15320 https://hg.mozilla.org/mozilla-central/rev/20546b86221e https://hg.mozilla.org/mozilla-central/rev/6eae71b641b2 https://hg.mozilla.org/mozilla-central/rev/0d6754739bf9 https://hg.mozilla.org/mozilla-central/rev/8bae4384ab4c https://hg.mozilla.org/mozilla-central/rev/109933956ea9 https://hg.mozilla.org/mozilla-central/rev/2b300e277494 https://hg.mozilla.org/mozilla-central/rev/70d1fd2ca5c6 https://hg.mozilla.org/mozilla-central/rev/bedd95bcb798 https://hg.mozilla.org/mozilla-central/rev/206bd2d6cda4 https://hg.mozilla.org/mozilla-central/rev/75f301551f98
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•