Skip to content

tls: scope loop vars with let - #4853

Closed
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:tls-no-redeclare
Closed

tls: scope loop vars with let#4853
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:tls-no-redeclare

Conversation

@Trott

@TrottTrott commented Jan 25, 2016

Copy link
Copy Markdown
Member

lib/_tls_common.js had instances of for loops that defined variables
with var such that they were re-declared in the same scope. This
change scopes those variables with let so that they are not
re-declared.

@Trott
tls: scope loop vars with let
9f08f84
`lib/_tls_common.js` had instances of `for` loops that defined variables
with `var` such that they were re-declared in the same scope. This
change scopes those variables with `let` so that they are not
re-declared.
@TrottTrott added the tls Issues and PRs related to the tls subsystem. label Jan 25, 2016
@ChALkeR

ChALkeR commented Jan 25, 2016

Copy link
Copy Markdown
Member

Why wasn't this catched by the linter?

@Trott

Trott commented Jan 25, 2016

Copy link
Copy Markdown
MemberAuthor

@ChALkeR We don't (yet) have the no-redeclare rule turned on.

In the current code base, that rule fires 227 times.

While that may or may not be an acceptable amount of churn for enabling the rule, I'm not so sure that the fixes don't require some examination and thought for each one. It's not always as simple as "oh, just change it from var to const" or "declare it once at the top" or whatever. Or at least sometimes "declare it once at the top" isn't always the best solution. (This change, in this PR, is an example of that, I think. It makes sense for the variables to be scoped and not hoisted here.) So, I'm doing little digestible bits and pieces.

@Trott

Trott commented Jan 25, 2016

Copy link
Copy Markdown
MemberAuthor

@ChALkeR If it's useful at all, here's the results of turning on no-redeclare on current master: https://gist.github.com/Trott/7dc16adf550b6a8d06cc

@Trott

Trott commented Jan 25, 2016

Copy link
Copy Markdown
MemberAuthor

CI: https://ci.nodejs.org/job/node-test-pull-request/1367/

@ChALkeR

ChALkeR commented Jan 25, 2016

Copy link
Copy Markdown
Member

@Trott Ah, thanks!

@cjihrig

cjihrig commented Jan 25, 2016

Copy link
Copy Markdown
Contributor

LGTM

@kthelgasonkthelgason mentioned this pull request Jan 25, 2016
Closed
@silverwind

silverwind commented Jan 26, 2016

Copy link
Copy Markdown
Contributor

LGTM, also cancelled that stuck CI job.

silverwind pushed a commit that referenced this pull request Jan 26, 2016
@Trott@silverwind
tls: scope loop vars with let
6a73dba
`lib/_tls_common.js` had instances of `for` loops that defined variables
with `var` such that they were re-declared in the same scope. This
change scopes those variables with `let` so that they are not
re-declared.
PR-URL: #4853
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
@silverwind

silverwind commented Jan 26, 2016

Copy link
Copy Markdown
Contributor

Thanks! Landed in 6a73dba.

rvagg pushed a commit that referenced this pull request Jan 27, 2016
@Trott@rvagg
tls: scope loop vars with let
db3135d
`lib/_tls_common.js` had instances of `for` loops that defined variables
with `var` such that they were re-declared in the same scope. This
change scopes those variables with `let` so that they are not
re-declared.
PR-URL: #4853
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
benjamingr pushed a commit to benjamingr/io.js that referenced this pull request Jan 27, 2016
@Trott@benjamingr
tls: scope loop vars with let
5d8a742
`lib/_tls_common.js` had instances of `for` loops that defined variables
with `var` such that they were re-declared in the same scope. This
change scopes those variables with `let` so that they are not
re-declared.
PR-URL: nodejs#4853
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
rvagg pushed a commit that referenced this pull request Feb 8, 2016
@Trott@rvagg
tls: scope loop vars with let
62479e3
`lib/_tls_common.js` had instances of `for` loops that defined variables
with `var` such that they were re-declared in the same scope. This
change scopes those variables with `let` so that they are not
re-declared.
PR-URL: #4853
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
@rvaggrvagg mentioned this pull request Feb 8, 2016
Closed
MylesBorins pushed a commit that referenced this pull request Feb 17, 2016
@TrottMyles Borins
tls: scope loop vars with let
114e345
`lib/_tls_common.js` had instances of `for` loops that defined variables
with `var` such that they were re-declared in the same scope. This
change scopes those variables with `let` so that they are not
re-declared.
PR-URL: #4853
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
MylesBorins pushed a commit that referenced this pull request Feb 18, 2016
@TrottMyles Borins
tls: scope loop vars with let
c0f5f01
`lib/_tls_common.js` had instances of `for` loops that defined variables
with `var` such that they were re-declared in the same scope. This
change scopes those variables with `let` so that they are not
re-declared.
PR-URL: #4853
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
@MylesBorinsMylesBorins mentioned this pull request Feb 18, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
@TrottMyles Borins
tls: scope loop vars with let
67be41a
`lib/_tls_common.js` had instances of `for` loops that defined variables
with `var` such that they were re-declared in the same scope. This
change scopes those variables with `let` so that they are not
re-declared.
PR-URL: #4853
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
@TrottMichael Scovetta
tls: scope loop vars with let
c445692
`lib/_tls_common.js` had instances of `for` loops that defined variables
with `var` such that they were re-declared in the same scope. This
change scopes those variables with `let` so that they are not
re-declared.
PR-URL: nodejs#4853
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
@Trott
Trott deleted the tls-no-redeclare branch January 13, 2022 22:32
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

tlsIssues and PRs related to the tls subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@Trott@ChALkeR@cjihrig@silverwind@jasnell@MylesBorins