From ccfd2b14ddd449ff403236dfe3ac56e8fa137a76 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Mon, 15 Jan 2018 02:51:36 +0800 Subject: [PATCH 1/3] doc: suggest not to throw JS errors from C++ Also provide an example on how to use internal/errors to handle errors in C++. --- CPP_STYLE_GUIDE.md | 65 +++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 59 insertions(+), 6 deletions(-) diff --git a/CPP_STYLE_GUIDE.md b/CPP_STYLE_GUIDE.md index 6266ee03b7c538..7e494daf162a4d 100644 --- a/CPP_STYLE_GUIDE.md +++ b/CPP_STYLE_GUIDE.md @@ -20,7 +20,8 @@ * [Others](#others) * [Type casting](#type-casting) * [Do not include `*.h` if `*-inl.h` has already been included](#do-not-include-h-if--inlh-has-already-been-included) - * [Avoid throwing JavaScript errors in nested C++ methods](#avoid-throwing-javascript-errors-in-nested-c-methods) + * [Avoid throwing JavaScript errors in C++ methods](#avoid-throwing-javascript-errors-in-c) + * [Avoid throwing JavaScript errors in nested C++ methods](#avoid-throwing-javascript-errors-in-nested-c-methods) Unfortunately, the C++ linter (based on [Google’s `cpplint`](https://github.com/google/styleguide)), which can be run @@ -213,12 +214,64 @@ instead of #include "util-inl.h" ``` -## Avoid throwing JavaScript errors in nested C++ methods +## Avoid throwing JavaScript errors in C++ -If you need to throw JavaScript errors from a C++ binding method, try to do it -at the top level and not inside of nested calls. +When there is a need to throw errors from a C++ binding method, try to +return the data necessary for constructing the errors to JavaScript, +then construct and throw the errors [using `lib/internal/errors.js`][errors]. -A lot of code inside Node.js is written so that typechecking etc. is performed -in JavaScript. +Note that in general, type-checks on arguments should done in JavaScript +before the arguments are passed into C++. + +If the return value of the binding cannot be used to signal failures or return +the necessary data for constructing errors in JavaScript, pass a context object +to the binding and put the necessary data inside in C++. For example: + +```c++ +void Foo(const FunctionCallbackInfo& args) { + Environment* env = Environment::GetCurrent(args); + // Let the JavaScript handle the actual type-checking, + // only assertions are placed in C++ + CHECK_EQ(args.Length(), 2); + CHECK(args[0]->IsString()); + CHECK(args[1]->IsObject()); + + int err = DoSomethingWith(args[0].As()); + if (err) { + // Put the data inside the error context + Local ctx = args[1].As(); + Local key = FIXED_ONE_BYTE_STRING(env->isolate(), "code") + ctx->Set(env->context(), key, err); + } else { + args.GetReturnValue().Set(something_to_return); + } +} + +// In the initialize function +env->SetMethod(target, "foo", Foo); +``` + +```js +exports.foo = function(str) { + // Prefer doing the type-checks in JavaScript + if (typeof str !== 'string') { + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'str', 'string'); + } + + const ctx = {}; + const result = binding.foo(str, ctx); + if (ctx.code !== undefined) { + throw errors.Error('ERR_ERROR_NAME', ctx.code); + } + return result; +} +``` + +### Avoid throwing JavaScript errors in nested C++ methods + +When you have to throw the errors from C++, try to do it at the top level and +not inside of nested calls. Using C++ `throw` is not allowed. + +[errors]: https://github.com/nodejs/node/blob/master/doc/guides/using-internal-errors.md From 877afe10649e1c6dac06b408513a83e374db24da Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Mon, 15 Jan 2018 03:23:25 +0800 Subject: [PATCH 2/3] [squash] fix typo --- CPP_STYLE_GUIDE.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CPP_STYLE_GUIDE.md b/CPP_STYLE_GUIDE.md index 7e494daf162a4d..7fc01dca40889e 100644 --- a/CPP_STYLE_GUIDE.md +++ b/CPP_STYLE_GUIDE.md @@ -220,7 +220,7 @@ When there is a need to throw errors from a C++ binding method, try to return the data necessary for constructing the errors to JavaScript, then construct and throw the errors [using `lib/internal/errors.js`][errors]. -Note that in general, type-checks on arguments should done in JavaScript +Note that in general, type-checks on arguments should be done in JavaScript before the arguments are passed into C++. If the return value of the binding cannot be used to signal failures or return From d42a2b58b272e31c7eb92de462fc52fd3bf3caa1 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Mon, 15 Jan 2018 23:42:30 +0800 Subject: [PATCH 3/3] [squash] mention C++ CHECKs, fix examples --- CPP_STYLE_GUIDE.md | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/CPP_STYLE_GUIDE.md b/CPP_STYLE_GUIDE.md index 7fc01dca40889e..998ad6983e6413 100644 --- a/CPP_STYLE_GUIDE.md +++ b/CPP_STYLE_GUIDE.md @@ -221,13 +221,14 @@ return the data necessary for constructing the errors to JavaScript, then construct and throw the errors [using `lib/internal/errors.js`][errors]. Note that in general, type-checks on arguments should be done in JavaScript -before the arguments are passed into C++. +before the arguments are passed into C++. Then in the C++ binding, simply using +`CHECK` assertions to guard against invalid arguments should be enough. If the return value of the binding cannot be used to signal failures or return the necessary data for constructing errors in JavaScript, pass a context object to the binding and put the necessary data inside in C++. For example: -```c++ +```cpp void Foo(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); // Let the JavaScript handle the actual type-checking, @@ -240,8 +241,8 @@ void Foo(const FunctionCallbackInfo& args) { if (err) { // Put the data inside the error context Local ctx = args[1].As(); - Local key = FIXED_ONE_BYTE_STRING(env->isolate(), "code") - ctx->Set(env->context(), key, err); + Local key = FIXED_ONE_BYTE_STRING(env->isolate(), "code"); + ctx->Set(env->context(), key, err).FromJust(); } else { args.GetReturnValue().Set(something_to_return); } @@ -261,10 +262,10 @@ exports.foo = function(str) { const ctx = {}; const result = binding.foo(str, ctx); if (ctx.code !== undefined) { - throw errors.Error('ERR_ERROR_NAME', ctx.code); + throw new errors.Error('ERR_ERROR_NAME', ctx.code); } return result; -} +}; ``` ### Avoid throwing JavaScript errors in nested C++ methods