src: handle empty Maybe(Local) in node_util.cc - #33867
Conversation
Sorry, something went wrong.
| if (!args[1]->Uint32Value(env->context()).To(&index)) return; | ||
| Local<Private> private_symbol = IndexToPrivateSymbol(env, index); | ||
| Local<Value> ret; | ||
| if (obj->GetPrivate(env->context(), private_symbol).ToLocal(&ret)) |
There was a problem hiding this comment.
I'm not 100% sure but I don't think GetPrivate() can actually fail provided that obj->IsObject().
Sorry, something went wrong.
|
@bnoordhuis The point isn’t so much as to whether these ops can currently fail or not, it’s more that I want to move more of our code into a state where it is “obviously not broken”, and be consistent about that. At least the SetPrivate() call can fail, and if we didn’t use .ToLocalChecked() & friends as excessively as we currently do, we probably wouldn’t have used it for that call either. |
Sorry, something went wrong.
|
We'll have to disagree on what "obviously not broken" means. Replacing CHECK(args[1]->IsUint32()) with a (recoverable) JS exception just makes it harder to diagnose bugs, IMO. Point ceded on the SetPrivate() call, although I'd still like people to report it when it fails, because that's likely a bug we need to fix. (You can probably trigger it if you go out of your way to poison the runtime but normally I would expect it to be a bug on our side.) |
Sorry, something went wrong.
Like a lot of operations, it fails when JS execution is being terminated. That’s one of the main points here – it is only obvious that .ToLocalChecked() doesn’t fail in very few cases, so it’s better to avoid it as much as possible. |
Sorry, something went wrong.
|
If TerminateExecution() compatibility is the goal here, why don't you mention that in the commit log? That would have avoided this back and forth. My point about changing the CHECK remains though: it serves no point that I can see. |
Sorry, something went wrong.
Because I thought it was obvious, sorry. I’ll add Refs: https://github.com/nodejs/node/blob/master/src/README.md#checked-conversion for PRs like this in the future.
Okay, since you feel strongly about it, I’ve restored it, and switched to the variant that we use when we CHECK() the type (i.e. .As<Uint32>()->Value() instead of ->Uint32Value(…)), as consistency is half of the point of this PR. |
Sorry, something went wrong.
Sorry, something went wrong.
Sorry, something went wrong.
PR-URL: #33867 Refs: https://github.com/nodejs/node/blob/master/src/README.md#checked-conversion Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #33867 Refs: https://github.com/nodejs/node/blob/master/src/README.md#checked-conversion Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #33867 Refs: https://github.com/nodejs/node/blob/master/src/README.md#checked-conversion Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #33867 Refs: https://github.com/nodejs/node/blob/master/src/README.md#checked-conversion Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #33867 Refs: https://github.com/nodejs/node/blob/master/src/README.md#checked-conversion Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #33867 Refs: https://github.com/nodejs/node/blob/master/src/README.md#checked-conversion Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
Refs: https://github.com/nodejs/node/blob/master/src/README.md#checked-conversion
Checklist