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.

Sign up for GitHub

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

Jump to bottom

debugger: remove variable redeclarations #4633

Closed
wants to merge 2 commits into from
Closed

debugger: remove variable redeclarations #4633

wants to merge 2 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Jan 11, 2016

Some variables are declared with var more than once in the same scope. This change reduces the declarations to one per scope.

@Trott Trott added the debugger label Jan 11, 2016
@cjihrig
Copy link
Contributor

cjihrig commented Jan 12, 2016

I didn't look at the entire file. Would any of these changes work as let instead of hoisting the vars? If so, we should do that. If not, LGTM.

@Trott
Copy link
Member Author

Trott commented Jan 12, 2016

I dismissed let out of hand on the grounds that there have been performance concerns around it in the past. Is that a case of throwing out the baby with the bath-water? If so, then there's probably some appropriate let opportunities in here.

@cjihrig
Copy link
Contributor

cjihrig commented Jan 12, 2016

We're using let in other places throughout the codebase. If I understand correctly, let is only problematic in a few edge cases, and likely to continue improving. The debugger also isn't a hot code path.

@jasnell
Copy link
Member

jasnell commented Jan 12, 2016

LGTM if CI and @cjihrig are happy :-)

@Trott
Copy link
Member Author

Trott commented Jan 13, 2016

OK, definitely an option for a let here and a couple of const declarations there. Updated. PTAL.

CI: https://ci.nodejs.org/job/node-test-commit/1714/

@jasnell
Copy link
Member

jasnell commented Jan 13, 2016

Still LGTM

@cjihrig
Copy link
Contributor

cjihrig commented Jan 13, 2016

LGTM too.

@Trott
Copy link
Member Author

Trott commented Jan 16, 2016

CI is green! https://ci.nodejs.org/job/node-test-pull-request/1273/

Trott added a commit to Trott/io.js that referenced this pull request Jan 16, 2016
@Trott
debugger: remove variable redeclarations
66b9c0d 
Some variables are declared with var more than once in the same scope.
This change reduces the declarations to one per scope.

PR-URL: nodejs#4633
Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@Trott
Copy link
Member Author

Trott commented Jan 16, 2016

Landed in 66b9c0d

@Trott Trott closed this Jan 16, 2016
@evanlucas evanlucas mentioned this pull request Jan 18, 2016
Merged
evanlucas pushed a commit that referenced this pull request Jan 18, 2016
@Trott @evanlucas
debugger: remove variable redeclarations
0d0a5ed 
Some variables are declared with var more than once in the same scope.
This change reduces the declarations to one per scope.

PR-URL: #4633
Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 28, 2016
@Trott
debugger: remove variable redeclarations
6030faa 
Some variables are declared with var more than once in the same scope.
This change reduces the declarations to one per scope.

PR-URL: #4633
Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 11, 2016
@Trott
debugger: remove variable redeclarations
587afee 
Some variables are declared with var more than once in the same scope.
This change reduces the declarations to one per scope.

PR-URL: #4633
Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 11, 2016
@Trott
debugger: remove variable redeclarations
7e3c6a7 
Some variables are declared with var more than once in the same scope.
This change reduces the declarations to one per scope.

PR-URL: nodejs#4633
Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 11, 2016
Merged
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 15, 2016
@Trott
debugger: remove variable redeclarations
583f334 
Some variables are declared with var more than once in the same scope.
This change reduces the declarations to one per scope.

PR-URL: nodejs#4633
Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
@Trott
debugger: remove variable redeclarations
307e0a5 
Some variables are declared with var more than once in the same scope.
This change reduces the declarations to one per scope.

PR-URL: nodejs#4633
Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@Trott Trott deleted the redecl-debugger branch January 13, 2022 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Trott @cjihrig @jasnell @MylesBorins