Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove old feature switches #86

Merged
merged 16 commits into from
Feb 11, 2016

Conversation

ianwjhalliday
Copy link
Collaborator

Addresses #6. Though there are a few more switches remain that I believe would be good to remove.

Removes the following switches:

  • LetConst
  • Map
  • Set
  • WeakMap
  • ES6WeakSet
  • DefineGetterSetter
  • __proto__
  • ES6Iterators
  • ES6Lambda
  • ES6NumericLiteral
  • ES6StringTemplate
  • ES6Symbol
  • TDZ

With the removal of -LetConst a couple of predicates become always true and have been removed. They are:

  • BindDeferredPidRefs()
  • IsBlockScopeEnabled()

Parser binding had specific logic to handle catch scopes in non-block scope mode to support legacy ES5 behavior. This has also been removed.

Finally Parser::CheckStrictModeFncDeclNotSourceElement() no longer did anything so it has also been removed.

@msftclas
Copy link

Hi @ianwjhalliday, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!


It looks like you're a Microsoft contributor (Ian Halliday). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

@ianwjhalliday
Copy link
Collaborator Author

@pleath appreciate you taking a look, the last few commits in particular.

FYI @tcare @boingoing @aneeshdk @akroshg @suwc @goyakin @abchatra


typeHandler->Convert(objectConstructor, mode, propertyCount);
typeHandler->Convert(objectConstructor, mode, 20);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we seize the opportunity to replace "20" with a meaningfully-named constant?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered that but all these Initialize functions use unnamed constants, and when you look at the parameter name its obvious what the value is for. I decided it wasn't worth bothering with.

@abchatra
Copy link
Contributor

It would be nice to keep -ES6Promise, ES6Unscopables, ES6String and ES6Object switches. These API's are frequently overridden by various polyfill's. An easy way to disable these comes in handy for website reductions.

@ianwjhalliday
Copy link
Collaborator Author

@abchatra good catch. I'll update the PR to leave out those removals.

@@ -8,7 +8,7 @@ namespace Js
{
ObjectPrototypeObject::ObjectPrototypeObject(DynamicType* type) : DynamicObject(type)
{
__proto__Enabled = GetScriptContext()->GetConfig()->Is__proto__Enabled();
__proto__Enabled = true; // TODO[ianhall]: Does this still apply if __proto__ is now always enabled? Check with JC
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we can now eliminate the flag altogether.

@abchatra
Copy link
Contributor

Any update here?

@abchatra
Copy link
Contributor

abchatra commented Feb 9, 2016

Any update here?

@ianwjhalliday ianwjhalliday force-pushed the rmfeatureswitches branch 3 times, most recently from 0a2f9a8 to e5ab1b8 Compare February 11, 2016 00:57
Ian Halliday added 14 commits February 10, 2016 17:31
Re issue chakra-core#6

Also add ThreadConfigFlagsList.h to the VS solution.
Re issue chakra-core#6

Also fixed up initialization of Array.prototype.values function.
After removal of the LetConst switch this predicate would only ever be
true.
After removal of the LetConst switch this predicate is always true.
Was used when block scoping was disabled which we no longer support.
Block scoping is now always enabled, so this catch scope behavior is
dead.
Function no longer does anything given ES6 block scoping.
Ian Halliday added 2 commits February 10, 2016 18:01
-TDZ feature switch was originally put in because temporal dead zones as
a concept were not yet decided upon for ES6 back in the day.  That ship
has long since sailed now and TDZ is here to stay.  Removing it.
@ianwjhalliday
Copy link
Collaborator Author

@dotnet-bot test Windows x64_debug please

@chakrabot chakrabot merged commit 620325c into chakra-core:master Feb 11, 2016
chakrabot pushed a commit that referenced this pull request Feb 11, 2016
Merge pull request #86 from ianwjhalliday:rmfeatureswitches
Addresses #6.  Though there are a few more switches remain that I believe would be good to remove.

Removes the following switches:
 - `LetConst`
 - `Map`
 - `Set`
 - `WeakMap`
 - `ES6WeakSet`
 - `DefineGetterSetter`
 - `__proto__`
 - `ES6Iterators`
 - `ES6Lambda`
 - `ES6NumericLiteral`
 - `ES6StringTemplate`
 - `ES6Symbol`
 - `TDZ`

With the removal of `-LetConst` a couple of predicates become always true and have been removed. They are:
 - `BindDeferredPidRefs()`
 - `IsBlockScopeEnabled()`

Parser binding had specific logic to handle catch scopes in non-block scope mode to support legacy ES5 behavior.  This has also been removed.

Finally `Parser::CheckStrictModeFncDeclNotSourceElement()` no longer did anything so it has also been removed.
@ianwjhalliday ianwjhalliday deleted the rmfeatureswitches branch February 11, 2016 03:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants