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)

enhancement

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.
Priority: -- → P3
Morph this bug into a more general clean-up issue.
Summary: Remove FullParseHandler::setComprehensionLambdaBody → Remove unused functions and clean-up various calls
- Remove some unused functions and variables.
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Attachment #8965514 - Flags: review?(jorendorff)
- 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)
- Remove the (basically) unused ValueArray and JSValueArray classes.
- (Passing raw |Value*| in ComputeThis seems a bit questionable...)
Attachment #8965516 - Flags: review?(jorendorff)
- 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)
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)
- 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 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+
Attachment #8965515 - Flags: review?(jorendorff) → review+
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+
Attachment #8965517 - Flags: review?(jorendorff) → review+
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)
Prefer UniqueChars/UniqueTwoByteChars over UniquePtr<char[], JS::FreePolicy> resp. UniquePtr<char16_t[], JS::FreePolicy>.
Attachment #8966273 - Flags: review?(jorendorff)
These four JSWhyMagic constants seem to be unused now.
Attachment #8966274 - Flags: review?(jorendorff)
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 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".
(In reply to Jeff Walden [:Waldo] from comment #14)
> MOZ_IS_GCC will work for this, with "mozilla/Compiler.h".

Thanks! :-)
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)
Use MOZ_IS_GCC in two more files.
Attachment #8966351 - Flags: review?(jwalden+bmo)
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+
(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
Attachment #8966351 - Flags: review?(jwalden+bmo) → review+
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+
Attachment #8966271 - Flags: review?(jorendorff) → review+
Attachment #8966273 - Flags: review?(jorendorff) → review+
Attachment #8966274 - Flags: review?(jorendorff) → review+
Attachment #8966350 - Flags: review?(jorendorff) → review+
Removes the unused HAVE_HYPOT definition from old-configure.in and updates some comments for Math.hypot.
Attachment #8967542 - Flags: review?(jorendorff)
More unused declarations which can be removed.
Attachment #8967543 - Flags: review?(jorendorff)
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)
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)
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)
Attachment #8967542 - Flags: review?(jorendorff) → review+
Attachment #8967543 - Flags: review?(jorendorff) → review+
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+
Attachment #8967554 - Flags: review?(jorendorff) → review+
Attachment #8967557 - Flags: review?(jorendorff) → review+
(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. :-)
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
You need to log in before you can comment on or make changes to this bug.