Skip to content

fs: remove needless assignment of null #10260

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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>
# for free to join this conversation on GitHub. Already have an account? # 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