pmed / v8pp

Bind C++ functions and classes into V8 JavaScript engine
http://pmed.github.io/v8pp/
Other
901 stars 121 forks source link

Refactored throw_ex function #90

Closed atvise closed 6 years ago

atvise commented 6 years ago
pmed commented 6 years ago

Hi,

Thanks for the contribution.

A question: what benefit of having two function overloads instead of one function with default argument?

And I probably need to comment that MultiByteToWideChar() usage. It's been used to convert an exception message from active code page encoding to UTF-8, because implementation of std::system_error stores an error message in ACP encoding in Visual C++ (at least in Visual C++ 2015).

https://blogs.msmvps.com/gdicanio/2017/08/16/what-is-the-encoding-used-by-the-error_code-message-string/

atvise commented 6 years ago

A question: what benefit of having two function overloads instead of one function with default argument?

Because I can't call throw_ex without it and the v8::Exception::Error etc functions will not make usage of the given Isolate parameter and call v8::Isolate::GetCurrent() internally. I have linked the v8 as a static library and want to have .dll addons therefore I can't call functions which make use of v8::Isolate::GetCurrent(). This functions will also be depricated in the future because the v8 people are refactoring them -> See e.g.: v8.h 4980 /** 4980 * Creates an error message for the given exception. 4981 * Will try to reconstruct the original stack trace from the exception value, 4982 * or capture the current stack trace if not available. 4983 */ 4984 static Local<Message> CreateMessage(Isolate* isolate, Local<Value> exception); 4985 V8_DEPRECATED("Use version with an Isolate*", 4986 static Local<Message> CreateMessage(Local<Value> exception));

Futhermore I don't know why we even have to call those v8::Exeption::Error etc. functions in the first place. V8 internally throwing errors will be made like: https://chromium.googlesource.com/v8/v8/+/5.9.85/samples/shell.cc 164 args.GetIsolate()->ThrowException( v8::String::NewFromUtf8(args.GetIsolate(), "Bad parameters", v8::NewStringType::kNormal).ToLocalChecked());

And I probably need to comment that MultiByteToWideChar() usage. It's been used to convert an exception message from active code page encoding to UTF-8, because implementation of std::system_error stores an error message in ACP encoding in Visual C++ (at least in Visual C++ 2015).

Ok I understand that but I can't see a usage of std::system error in your code examples or implementation. I would suggest a function which will convert those std::system_error messages to v8::Values and have a third throw_ex function which will thake those v8::Values ?

pmed commented 6 years ago

Hi,

Thanks for the PR, I've merged these changes.

A note for me in the future: error message conversion of std::system_error from ACP to UTF-8 is out of scope this library and should be performed in a client code.

atvise commented 6 years ago

Thank you very much!