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

fs: remove needless assignment of null #10260

Closed
wants to merge 1 commit into from

Conversation

reconbot
Copy link
Contributor

@reconbot reconbot commented Dec 13, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

streams

Description of change

This line pool = null; isn't needed and has been around since the first iteration of fs.stream. I can't find a good reason for it to exist, it's not more readable, nor does it seem to trick the compiler into any optimizations.

@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. lts-watch-v6.x labels Dec 13, 2016
@addaleax
Copy link
Member

Looks good but could you edit the commit message & PR title so that it starts with fs:?

This line `pool = null;` isn't needed and has
been around since the first iteration of streams.
I can't find a good reason for it to exist, it's
not more readable, nor does it seem to trick the
compiler into any optimizations.
@reconbot reconbot changed the title streams: remove needless assignment of null fs: remove needless assignment of null Dec 13, 2016
@MylesBorins
Copy link
Contributor

/cc @nodejs/streams

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM

(contrary to popular belief, this is not a streams change :P)

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM even it it is not a stream change :D.

@reconbot
Copy link
Contributor Author

Is there a @nodejs/fs group to cc?

@gibfahn
Copy link
Member

gibfahn commented Dec 18, 2016

cc/ @nodejs/fs

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

Seems straightforward. The first thing allocNewPool() does is assign pool a new value.

@italoacasas
Copy link
Contributor

@italoacasas
Copy link
Contributor

Landed 1081f0f

cjihrig pushed a commit to cjihrig/node that referenced this pull request Dec 20, 2016
This line `pool = null;` isn't needed and has
been around since the first iteration of streams.
I can't find a good reason for it to exist, it's
not more readable, nor does it seem to trick the
compiler into any optimizations.

PR-URL: nodejs#10260
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
@italoacasas italoacasas mentioned this pull request Dec 20, 2016
cjihrig pushed a commit that referenced this pull request Dec 20, 2016
This line `pool = null;` isn't needed and has
been around since the first iteration of streams.
I can't find a good reason for it to exist, it's
not more readable, nor does it seem to trick the
compiler into any optimizations.

PR-URL: #10260
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
@reconbot reconbot deleted the streams-null branch December 22, 2016 16:50
MylesBorins pushed a commit that referenced this pull request Jan 23, 2017
This line `pool = null;` isn't needed and has
been around since the first iteration of streams.
I can't find a good reason for it to exist, it's
not more readable, nor does it seem to trick the
compiler into any optimizations.

PR-URL: #10260
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
This line `pool = null;` isn't needed and has
been around since the first iteration of streams.
I can't find a good reason for it to exist, it's
not more readable, nor does it seem to trick the
compiler into any optimizations.

PR-URL: #10260
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2017
MylesBorins pushed a commit that referenced this pull request Jan 31, 2017
This line `pool = null;` isn't needed and has
been around since the first iteration of streams.
I can't find a good reason for it to exist, it's
not more readable, nor does it seem to trick the
compiler into any optimizations.

PR-URL: #10260
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants