Conversation
| CHECK(args.IsConstructCall()); | ||
| Environment* env = Environment::GetCurrent(args); | ||
| JSStream* wrap; | ||
| JSStream* wrap = NULL; |
There was a problem hiding this comment.
This isn't necessary, either the variable will already be assigned below or the process aborts because of an unreachable condition. Also, nullptr should be used anyway for C++.
|
I'm not sure all of this is worthwhile. |
| Local<Function> pre_fn = env()->async_hooks_pre_function(); | ||
| Local<Function> post_fn = env()->async_hooks_post_function(); | ||
| Local<Value> uid = Number::New(env()->isolate(), get_uid()); | ||
| Local<Value> uid = Number::New(env()->isolate(), static_cast<double>(get_uid())); |
There was a problem hiding this comment.
I would recommend against changing these things here, there would be quite a few conflicts with #11883
|
|
||
| int32_t flags = (args[3]->IsInt32()) ? args[3]->Int32Value() : 0; | ||
| int family; | ||
| int family = 0; |
There was a problem hiding this comment.
This seems confusing for the same reason as @mscdex’ comment below … :/
| session_size_ = 0; | ||
| session_id_ = nullptr; | ||
| tls_ticket_size_ = -1; | ||
| tls_ticket_size_ = static_cast<uint16_t>(-1); |
There was a problem hiding this comment.
👍 , this does increase readability a bit
|
|
||
| case UV_FS_OPEN: | ||
| argv[1] = Integer::New(env->isolate(), req->result); | ||
| argv[1] = Integer::New(env->isolate(), static_cast<int32_t>(req->result)); //possible los of data |
There was a problem hiding this comment.
typo: los (also: practically speaking there’s absolutely nothing to worry about, you could just leave the comment away)
|
ping @benmouh … are you interested in continuing this? |
|
Yes @addaleax i'm so still wondring why not already merged into the master |
The parts of this that fix compiler warnings are probably worthwhile. |
|
I'm yet start by some compiler warnings in this branch then for other code optimization will be in an other separated branch soon |
|
This needs a rebase. @benmouh do you want to follow up on this and also address the comments? |
|
Closing this due to long inactivity without addressing the comments. @benmouh thanks for your contribution anyway, it is much appreciated. Please also feel free to leave a comment if you want to follow up on this so we can reopen the issue or just open a new PR. |
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
I provided some code optimization in different part of the project :