src: make EnvDelete behave like the delete operator #7975
Conversation
nodejs-github-bot
added
the
c++
|
This changes observable behavior of |
| #ifdef __POSIX__ | ||
| node::Utf8Value key(info.GetIsolate(), property); | ||
| rc = getenv(*key) != nullptr; | ||
| if (rc) | ||
| bool has_key = getenv(*key) != nullptr; |
There was a problem hiding this comment.
You could fold this into the if statement.
|
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
| #ifdef __POSIX__ | ||
| node::Utf8Value key(info.GetIsolate(), property); | ||
| rc = getenv(*key) != nullptr; | ||
| if (rc) | ||
| if (getenv(*key) != nullptr) |
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.
|
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. Fixes nodejs#7960
|
Thanks! CI: https://ci.nodejs.org/job/node-test-commit/4398/ |
|
LGTM |
2 similar comments
|
LGTM |
|
LGTM |
|
@addaleax ... does this LGTY? |
|
Yes, LGTM! :) |
|
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 <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
|
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 <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
|
adding lts watch label. please let me know if this should be landed or not |
|
Not sure. My feeling is we don't need this fix in v4. |
|
moving to don't land thanks @fhinkel |
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