Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Refactor/code optimization #12187

Closed
wants to merge 4 commits into from
Closed

Refactor/code optimization #12187

wants to merge 4 commits into from

Conversation

benmouh
Copy link

@benmouh benmouh commented Apr 3, 2017

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows [commit guidelines][]
Affected core subsystem(s)

I provided some code optimization in different part of the project :

  • Supress some compiler warnings
  • Possible data loss on type conversion

@nodejs-github-bot nodejs-github-bot added async_wrap buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. crypto Issues and PRs related to the crypto subsystem. fs Issues and PRs related to the fs subsystem / file system. http_parser Issues and PRs related to the HTTP Parser dependency or the http_parser binding. os Issues and PRs related to the os subsystem. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Apr 3, 2017
mscdex
mscdex reviewed Apr 3, 2017
View reviewed changes
src/js_stream.cc
@@ -115,7 +115,7 @@ void JSStream::New(const FunctionCallbackInfo<Value>& args) {
// normal function.
CHECK(args.IsConstructCall());
Environment* env = Environment::GetCurrent(args);
JSStream* wrap;
JSStream* wrap = NULL;
Copy link
Contributor

@mscdex mscdex Apr 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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++.

@mscdex
Copy link
Contributor

mscdex commented Apr 3, 2017

I'm not sure all of this is worthwhile.

addaleax
addaleax reviewed Apr 3, 2017
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @mscdex, most of the changes here seem like they introduce redundancy where it’s not really necessary

src/async-wrap.cc
@@ -320,7 +320,7 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,

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()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend against changing these things here, there would be quite a few conflicts with #11883

src/cares_wrap.cc
@@ -1150,7 +1150,7 @@ static void GetAddrInfo(const FunctionCallbackInfo<Value>& args) {
node::Utf8Value hostname(env->isolate(), args[1]);

int32_t flags = (args[3]->IsInt32()) ? args[3]->Int32Value() : 0;
int family;
int family = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems confusing for the same reason as @mscdex’ comment below … :/

src/node_crypto_clienthello-inl.h
@@ -35,7 +35,7 @@ inline void ClientHelloParser::Reset() {
extension_offset_ = 0;
session_size_ = 0;
session_id_ = nullptr;
tls_ticket_size_ = -1;
tls_ticket_size_ = static_cast<uint16_t>(-1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 , this does increase readability a bit

src/node_file.cc
@@ -224,11 +224,11 @@ static void After(uv_fs_t *req) {
break;

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: los (also: practically speaking there’s absolutely nothing to worry about, you could just leave the comment away)

@addaleax
Copy link
Member

ping @benmouh … are you interested in continuing this?

@mscdex mscdex added lib / src Issues and PRs related to general changes in the lib or src directory. and removed async_wrap buffer Issues and PRs related to the buffer subsystem. cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. crypto Issues and PRs related to the crypto subsystem. fs Issues and PRs related to the fs subsystem / file system. http_parser Issues and PRs related to the HTTP Parser dependency or the http_parser binding. os Issues and PRs related to the os subsystem. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels May 28, 2017
@benmouh
Copy link
Author

benmouh commented May 30, 2017

Yes @addaleax i'm so still wondring why not already merged into the master

@gibfahn
Copy link
Member

gibfahn commented May 30, 2017 •
edited
Loading

Yes @addaleax i'm so still wondring why not already merged into the master

Did you see the code review from @addaleax and @mscdex ?

This also needs to be rebased against master, as it now conflicts.

@gibfahn
Copy link
Member

gibfahn commented May 30, 2017

I agree with @mscdex, most of the changes here seem like they introduce redundancy where it’s not really necessary

I'm not sure all of this is worthwhile.

The parts of this that fix compiler warnings are probably worthwhile.

@benmouh
Copy link
Author

benmouh commented May 31, 2017

I'm yet start by some compiler warnings in this branch then for other code optimization will be in an other separated branch soon

@BridgeAR
Copy link
Member

This needs a rebase. @benmouh do you want to follow up on this and also address the comments?

@jasnell jasnell added the stalled Issues and PRs that are stalled. label Aug 29, 2017
@BridgeAR
Copy link
Member

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.

@BridgeAR BridgeAR closed this Sep 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax left review comments

@mscdex mscdex mscdex left review comments

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. stalled Issues and PRs that are stalled.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@benmouh @mscdex @addaleax @gibfahn @BridgeAR @jasnell @nodejs-github-bot