v8: add type check to make failed calls visible #1652
Conversation
Fishrock123
added
the
c++
| @@ -60,6 +60,14 @@ void GetHeapStatistics(const FunctionCallbackInfo<Value>& args) { | |||
|
|
|||
|
|
|||
| void SetFlagsFromString(const FunctionCallbackInfo<Value>& args) { | |||
| Isolate* isolate = args.GetIsolate(); | |||
| Environment* env = Environment::GetCurrent(isolate); | |||
There was a problem hiding this comment.
You can use the slightly more efficient Environment::GetCurrent(args) method here.
Calling v8.setFlagsFromString with e.g a function as a flag argument gave no exception or warning that the function call will fail. There is now an exception if the function gets called with the wrong flag type (string is required) or that a flag is expected. Other APIs already provide exceptions if the argument has not the expected type.
| Environment* env = Environment::GetCurrent(args); | ||
|
|
||
| if (args.Length() < 1) | ||
| return env->ThrowTypeError("v8 flag is required"); |
There was a problem hiding this comment.
Would a RangeError not be more appropriate?
There was a problem hiding this comment.
Aren't RangeErrors for integer argument values that are not between some min and max value?
There was a problem hiding this comment.
Yes, that might be right, but args.Length() is an integer value that should be between 1 and infinity.
There was a problem hiding this comment.
args.Length returns the number of JavaScript arguments that were passed over. It should prevent, that no argument gets passed in. Other io.js APIs handle this special case with a TypeError, too. RangeError typically thrown if e.g. the process.uid is out of the possible range. My main thought was to keep it in sync with the other APIs.
There was a problem hiding this comment.
Alright, nevermind then. If the existing code uses TypeError then TypeError it is.
Calling v8.setFlagsFromString with e.g a function as a flag argument gave no exception or warning that the function call will fail. There is now an exception if the function gets called with the wrong flag type (string is required) or that a flag is expected. Other APIs already provide exceptions if the argument has not the expected type. PR-URL: #1652 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
|
LGTM, thanks; landed in 931a0d4. I changed the commit log a little because |
Calling v8.setFlagsFromString with e.g a function as a flag argument gave no exception or warning that the function call will fail. There is now an exception if the function gets called with the wrong flag type (string is required) or that a flag is expected. Other APIs already provide exceptions if the argument has not the expected type. PR-URL: nodejs#1652 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Calling v8.setFlagsFromString with e.g a function as a flag argument gave no exception or warning that the function call will fail. There is now an exception if the function gets called with the wrong flag type (string is required) or that a flag is expected. v8 flags are only possible as strings.
Other APIs (
fsornet) already provide exceptions if the argument has not the expected type.