Skip to content

Revert ES3/ES5 downlevel computed property emit logic to not use tree rewriting#2124

Merged
DanielRosenwasser merged 3 commits into
masterfrom
unrewrite
Apr 8, 2015
Merged

Revert ES3/ES5 downlevel computed property emit logic to not use tree rewriting#2124
DanielRosenwasser merged 3 commits into
masterfrom
unrewrite

Conversation

@DanielRosenwasser
Copy link
Copy Markdown
Member

Fixes #2586

@JsonFreeman
Copy link
Copy Markdown
Contributor

👍

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

Hold off on this for just a bit. I'd like to investigate some of the numbers around perf (both CPU and Memory).

@ahejlsberg
Copy link
Copy Markdown
Member

@DanielRosenwasser We should get this one merged. No need to research perf.

Conflicts:
	src/compiler/emitter.ts
	tests/baselines/reference/ES5SymbolProperty1.js
	tests/baselines/reference/FunctionDeclaration8_es6.js
	tests/baselines/reference/FunctionDeclaration9_es6.js
	tests/baselines/reference/FunctionPropertyAssignments5_es6.js
	tests/baselines/reference/computedPropertyNames10_ES5.js
	tests/baselines/reference/computedPropertyNames11_ES5.js
	tests/baselines/reference/computedPropertyNames18_ES5.js
	tests/baselines/reference/computedPropertyNames19_ES5.js
	tests/baselines/reference/computedPropertyNames1_ES5.js
	tests/baselines/reference/computedPropertyNames20_ES5.js
	tests/baselines/reference/computedPropertyNames22_ES5.js
	tests/baselines/reference/computedPropertyNames23_ES5.js
	tests/baselines/reference/computedPropertyNames25_ES5.js
	tests/baselines/reference/computedPropertyNames26_ES5.js
	tests/baselines/reference/computedPropertyNames28_ES5.js
	tests/baselines/reference/computedPropertyNames29_ES5.js
	tests/baselines/reference/computedPropertyNames30_ES5.js
	tests/baselines/reference/computedPropertyNames31_ES5.js
	tests/baselines/reference/computedPropertyNames33_ES5.js
	tests/baselines/reference/computedPropertyNames34_ES5.js
	tests/baselines/reference/computedPropertyNames46_ES5.js
	tests/baselines/reference/computedPropertyNames47_ES5.js
	tests/baselines/reference/computedPropertyNames48_ES5.js
	tests/baselines/reference/computedPropertyNames49_ES5.js
	tests/baselines/reference/computedPropertyNames4_ES5.js
	tests/baselines/reference/computedPropertyNames50_ES5.js
	tests/baselines/reference/computedPropertyNames5_ES5.js
	tests/baselines/reference/computedPropertyNames6_ES5.js
	tests/baselines/reference/computedPropertyNames7_ES5.js
	tests/baselines/reference/computedPropertyNames8_ES5.js
	tests/baselines/reference/computedPropertyNames9_ES5.js
	tests/baselines/reference/computedPropertyNamesContextualType10_ES5.js
	tests/baselines/reference/computedPropertyNamesContextualType1_ES5.js
	tests/baselines/reference/computedPropertyNamesContextualType2_ES5.js
	tests/baselines/reference/computedPropertyNamesContextualType3_ES5.js
	tests/baselines/reference/computedPropertyNamesContextualType4_ES5.js
	tests/baselines/reference/computedPropertyNamesContextualType5_ES5.js
	tests/baselines/reference/computedPropertyNamesContextualType6_ES5.js
	tests/baselines/reference/computedPropertyNamesContextualType7_ES5.js
	tests/baselines/reference/computedPropertyNamesContextualType8_ES5.js
	tests/baselines/reference/computedPropertyNamesContextualType9_ES5.js
	tests/baselines/reference/computedPropertyNamesDeclarationEmit5_ES5.js
	tests/baselines/reference/computedPropertyNamesSourceMap2_ES5.js
	tests/baselines/reference/computedPropertyNamesSourceMap2_ES5.js.map
	tests/baselines/reference/computedPropertyNamesSourceMap2_ES5.sourcemap.txt
	tests/baselines/reference/parserES5ComputedPropertyName2.js
	tests/baselines/reference/parserES5ComputedPropertyName3.js
	tests/baselines/reference/parserES5ComputedPropertyName4.js
	tests/baselines/reference/privateIndexer2.js
@DanielRosenwasser
Copy link
Copy Markdown
Member Author

Since I just did a merge, can @mhegazy, @CyrusNajmabadi, or @JsonFreeman review/re-review this?

@JsonFreeman
Copy link
Copy Markdown
Contributor

I don't think there is a good way to review this. Just make sure all the tests are passing.

An alternative is to redo the merge, and commit without resolving conflicts, then resolve the conflicts as another commit. That way we can review the conflict resolution without reviewing the merge itself.

DanielRosenwasser added a commit that referenced this pull request Apr 8, 2015
Revert ES3/ES5 downlevel computed property emit logic to not use tree rewriting
@DanielRosenwasser DanielRosenwasser merged commit 927231b into master Apr 8, 2015
@DanielRosenwasser DanielRosenwasser deleted the unrewrite branch April 8, 2015 18:45
@microsoft microsoft locked and limited conversation to collaborators Jun 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Incorrect emit for computed accessors in object literals in ES3/ES5

6 participants