Skip to content

src: fix use of uninitialized variable - #9280

Closed
bnoordhuis wants to merge 2 commits into
nodejs:masterfrom
bnoordhuis:fix-uninitialized-variable
Closed

src: fix use of uninitialized variable#9280
bnoordhuis wants to merge 2 commits into
nodejs:masterfrom
bnoordhuis:fix-uninitialized-variable

Conversation

@bnoordhuis

@bnoordhuisbnoordhuis commented Oct 25, 2016

Copy link
Copy Markdown
Member

Fix the following compiler warning:
../src/node_i18n.cc: In function 'void node::i18n::GetStringWidth(
const v8::FunctionCallbackInfo<v8::Value>&)':
../src/node_i18n.cc:543:15: warning: 'c' may be used uninitialized
in this function [-Wmaybe-uninitialized]
if (!expand_emoji_sequence &&
~~~~~~~~~~~~~~~~~~~~~~~~~
n > 0 && p == 0x200d && // 0x200d == ZWJ (zero width join
Introduced in commit e8eaaa7 ("buffer: add buffer.transcode") from
earlier today.
@nodejs-github-botnodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 25, 2016

@addaleaxaddaleax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM but the commit message references the wrong commit (I think that should be 72547fe “readline: use icu based string width calculation”)

@bnoordhuis

Copy link
Copy Markdown
MemberAuthor

Aw, you're right. I'll fix that before landing, thanks.

jasnell
jasnell previously approved these changes Oct 25, 2016

@jasnelljasnell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@bnoordhuisbnoordhuis mentioned this pull request Oct 25, 2016
2 tasks
@jasnell
jasnell dismissed their stale reviewOctober 25, 2016 21:34

Actually... there's a problem with this

Comment threadsrc/node_i18n.cc
}

TwoByteValue value(env->isolate(), args[0]);
if (value.length() <= 1) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This additional shortcut is wrong. See the CI failures (https://ci.nodejs.org/job/node-test-commit-osx/5807/nodes=osx1010/console).

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I'm about to go to bed so if you want to take this over, go ahead.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1 ... I just went ahead and created a new PR. I'll close this one in favor of that and we can follow up there. Thank you @bnoordhuis !

@jasnelljasnell mentioned this pull request Oct 25, 2016
3 tasks
@jasnell

Copy link
Copy Markdown
Member

Closing in favor of #9281

@jasnelljasnell closed this Oct 25, 2016
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++Issues and PRs that require attention from people who are familiar with C++.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@bnoordhuis@jasnell@addaleax@nodejs-github-bot