-
-
Notifications
You must be signed in to change notification settings - Fork 85
new serialization for js <-> cpp interaction #2400
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Changes requested. Reviewed everything up to 9323adc in 2 minutes and 10 seconds
More details
- Looked at
227lines of code in4files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. serialization/include/archives/NapiOutputArchive.h:32
- Draft comment:
Avoid using 'static_assert(!sizeof(T), "not string")'. This always fails when instantiated. Consider using a dependent false idiom (e.g., 'static_assert(always_false::value, "not a string type")') to disable this overload via SFINAE. - Reason this comment was not posted:
Marked as duplicate.
2. skymp5-server/cpp/server_guest_lib/Inventory.cpp:200
- Draft comment:
Most vexing parse: 'NapiOutputArchive ar();' is interpreted as a function declaration rather than an object. Use 'NapiOutputArchive ar(env);' (or brace initialization) to properly instantiate the archive with the environment. - Reason this comment was not posted:
Marked as duplicate.
3. skymp5-server/cpp/server_guest_lib/Inventory.cpp:201
- Draft comment:
Usage of 'const_cast' to call Serialize() suggests that Serialize might modify the object. Consider revisiting the design to either allow Serialize() to be const if it only affects serialization state, or avoid const_cast to maintain const-correctness. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
4. skymp5-server/cpp/server_guest_lib/Inventory.cpp:200
- Draft comment:
Typographical issue: the declaration 'NapiOutputArchive ar();' in Inventory::ToNapiObject() is interpreted by the C++ compiler as a function declaration (due to the most vexing parse) instead of an object instantiation. It should be changed to 'NapiOutputArchive ar;' to correctly create an instance of NapiOutputArchive. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_TuXrSKOOzjgJHvG4
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| Napi::Value Inventory::ToNapiObject() const | ||
| { | ||
| auto env = jsEngine; | ||
| NapiOutputArchive ar(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid most vexing parse: 'NapiOutputArchive ar();' declares a function instead of instantiating an object. Use 'NapiOutputArchive ar(env);' instead.
| NapiOutputArchive ar(); | |
| NapiOutputArchive ar(env); |
| template <StringLike T> | ||
| NapiOutputArchive& Serialize(const T& input) | ||
| { | ||
| static_assert(!sizeof(T), "not string"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The static_assert in the StringLike overload always fails because sizeof(T) is never 0. Consider using a dependent false typetrick instead.
| static_assert(!sizeof(T), "not string"); | |
| static_assert(std::false_type::value, "not string"); |
|
Ой, там да, есть прикол с jsEngine. По другому env должен передаваться |
| NapiOutputArchive& Serialize(const char* key, const std::optional<T>& input) | ||
| { | ||
| if (input.has_value()) | ||
| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please format with clang-format
- 2 spaces for indents
{on the same line for if-s and for-s
| return *this; | ||
| } | ||
|
|
||
| Napi::Value extract_output() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep in line with the current conventions https://github.com/skyrim-multiplayer/skymp/blob/main/docs/docs_cpp_code_guidelines.md
| Napi::Value extract_output() | |
| Napi::Value ExtractOutput() |
|
|
||
| Napi::Env m_env; | ||
| std::optional<Napi::Value> m_outputOther; | ||
| std::optional<Napi::Object> m_outputObject; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, there is no rule to prefix member names m_
| return std::move(*m_outputObject); | ||
| } | ||
|
|
||
| throw std::runtime_error("Uninitialized field!"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'd also have to change this to more meaningful messages before merge. Feel free to keep it as it is while it's wip though
Important
Add
NapiOutputArchivefor C++ to JavaScript serialization and updateInventoryclass and CMake configuration accordingly.NapiOutputArchiveclass inNapiOutputArchive.hfor serializing C++ objects to JavaScript objects using Node-API.ToNapiObject()method inInventory.cppto serializeInventoryobjects to JavaScript objects usingNapiOutputArchive.NapiOutputArchive.hand<napi.h>inInventory.cppandInventory.h.CMakeLists.txtto find and linkunofficial-node-addon-api.This description was created by
for 9323adc. It will automatically update as commits are pushed.