src: make EnvDelete behave like the delete operator#7975
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added the c++ 
fhinkel commented Aug 4, 2016
This changes observable behavior of |
src/node.cc Outdated
There was a problem hiding this comment.
You could fold this into the if statement.
bnoordhuis commented Aug 4, 2016
Can you reword the commit log so it says "src: make EnvDelete etc etc"? As to whether this should be semver-major or not, I personally lean towards bug fix. |
fhinkel changed the title 
src/node.cc Outdated
There was a problem hiding this comment.
Is this check even necessary anymore?
There was a problem hiding this comment.
Good catch, according to unsetenv(3) no need for it. I'm assuming unsetenv() is not slower than doing the getenv() check first.
addaleax commented Aug 4, 2016
Got a question but otherwise this is looking good! |
According to TC39 specification, the delete operator returns false or throws in strict mode, if the property is non-configurable. It returns true in all other cases. Process.env can never have non-configurable properties, thus EnvDelete must always return true. This is independent of strict mode. Fixesnodejs#7960
addaleax commented Aug 4, 2016
Thanks! CI: https://ci.nodejs.org/job/node-test-commit/4398/ |
cjihrig commented Aug 4, 2016
LGTM |
2 similar comments
bnoordhuis commented Aug 4, 2016
LGTM |
jasnell commented Aug 4, 2016
LGTM |
jasnell commented Aug 5, 2016
@addaleax ... does this LGTY? |
addaleax commented Aug 5, 2016
Yes, LGTM! :) |
jasnell commented Aug 5, 2016
Awesome, thank you! will get this landed now. |
According to TC39 specification, the delete operator returns false or throws in strict mode, if the property is non-configurable. It returns true in all other cases. Process.env can never have non-configurable properties, thus EnvDelete must always return true. This is independent of strict mode. Fixes: #7960 PR-URL: #7975 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
jasnell commented Aug 5, 2016
Landed in ff7a841! thank you @fhinkel! |
According to TC39 specification, the delete operator returns false or throws in strict mode, if the property is non-configurable. It returns true in all other cases. Process.env can never have non-configurable properties, thus EnvDelete must always return true. This is independent of strict mode. Fixes: #7960 PR-URL: #7975 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins commented Sep 30, 2016
adding lts watch label. please let me know if this should be landed or not |
fhinkel commented Sep 30, 2016
Not sure. My feeling is we don't need this fix in v4. |
MylesBorins commented Sep 30, 2016
moving to don't land thanks @fhinkel |
7 participants
Checklist
make -j4 test(UNIX) passesdeletein the exampleAffected core subsystem(s)
src
Description of change
According to TC39 specification, the delete
operator returns false or throws
in strict mode, if the property is
non-configurable. It returns true in all other cases.
Process.env can never have non-configurable
properties, thus EnvDelete must always return true. This
is independent of strict mode.
Fixes#7960