Skip to content

debugger: remove variable redeclarations#4633

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
Closed
Trott wants to merge 2 commits into from
Closed

debugger: remove variable redeclarations #4633

Trott wants to merge 2 commits into from

Conversation

@Trott
Copy link
Member

@TrottTrott 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.

@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
MemberAuthor

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
MemberAuthor

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
MemberAuthor

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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
@Trott
Copy link
MemberAuthor

Trott commented Jan 16, 2016

Landed in 66b9c0d

@TrottTrott closed this Jan 16, 2016
@evanlucasevanlucas 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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
@MylesBorinsMylesBorins 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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
@TrottTrott deleted the redecl-debugger branch January 13, 2022 22:31
Sign up for freeto 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