FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: fix exception message encoding by mscdex · Pull Request #3288 · nodejs/node · GitHub

/ node Public

src: fix exception message encoding - #3288

Closed
mscdex wants to merge 1 commit into
nodejs:masterfrom
mscdex:fix-exception-encoding
ConversationCommits (1)ChecksFiles changed
Closed

src: fix exception message encoding#3288
mscdex wants to merge 1 commit into
nodejs:masterfrom
mscdex:fix-exception-encoding

Conversation

mscdex commented Oct 8, 2015

Copy link
Copy Markdown
Contributor

The printf family of functions do not properly display UTF8 strings well on Windows. Use the appropriate wide character API instead if stderr is a tty.

Fixes: #3284

mscdex added c++ Issues and PRs that require attention from people who are familiar with C++. windows Issues and PRs related to the Windows platform. labels Oct 8, 2015

mscdex commented Oct 8, 2015

Copy link
Copy Markdown
Contributor Author

CI: https://ci.nodejs.org/view/iojs/job/node-test-pull-request/461/

evanlucas commented Oct 9, 2015

Copy link
Copy Markdown
Contributor

Is it not necessary to add on https://github.com/nodejs/node/blob/master/src/node.cc#L3210?

bnoordhuis reviewed Oct 9, 2015
View reviewed changes
Comment thread src/node.cc Outdated

bnoordhuis Oct 9, 2015

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Style issue: const char*.

bnoordhuis commented Oct 9, 2015

Copy link
Copy Markdown
Member

This isn't complete yet, I think? There are a lot of (f)printf statements strewn throughout the code base.

mscdex commented Oct 9, 2015

Copy link
Copy Markdown
Contributor Author

@evanlucas That has to do with program arguments, but the string values are not from user input such that they could contain utf8.

@bnoordhuis Well, of the other printf()/fprintf() instances I could find, the string variables did not seem likely to contain utf8. If you had some specifically in mind, let me know.

mscdex force-pushed the fix-exception-encoding branch 2 times, most recently from 3bdc6b3 to 5d7e67a Compare October 9, 2015 17:02

mscdex commented Oct 9, 2015

Copy link
Copy Markdown
Contributor Author

CI again: https://ci.nodejs.org/view/iojs/job/node-test-pull-request/465/

silverwind commented Oct 12, 2015

Copy link
Copy Markdown
Contributor

CI is fine except the random ARM failures. I can confirm it fixes #3284.

bnoordhuis reviewed Oct 13, 2015
View reviewed changes
Comment thread src/node.cc Outdated

bnoordhuis Oct 13, 2015

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I'm kind of surprised this works, I thought vsnprintf() on Windows returned -1 when the output buffer is too small.

mscdex Oct 13, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I tested it on Windows 7 with VS2013 at least. I suppose _vscprintf() could be used instead...

bnoordhuis commented Oct 13, 2015

Copy link
Copy Markdown
Member

I'm going to defer to @nodejs/platform-windows.

piscisaureus commented Oct 13, 2015

Copy link
Copy Markdown
Contributor

Lgtm

src: fix exception message encoding
238d1ae 
The printf family of functions do not properly display UTF8
strings well on Windows. Use the appropriate wide character
API instead if stderr is a tty.

Fixes: nodejs#3284
mscdex force-pushed the fix-exception-encoding branch from 5d7e67a to 238d1ae Compare October 13, 2015 21:18

mscdex commented Oct 13, 2015

Copy link
Copy Markdown
Contributor Author

Final CI run after changing vsnprintf()->_vscprintf(): https://ci.nodejs.org/job/node-test-commit/830/

Only failures are an unrelated failure on Win10 and a recurring unrelated failure on FreeBSD 10.1 64-bit.

mscdex added a commit that referenced this pull request Oct 13, 2015
src: fix exception message encoding on Windows
2d35607 
The printf family of functions do not properly display UTF8
strings well on Windows. Use the appropriate wide character
API instead if stderr is a tty.

PR-URL: #3288
Fixes: #3284
Reviewed-By: Bert Belder <bertbelder@gmail.com>

mscdex commented Oct 13, 2015

Copy link
Copy Markdown
Contributor Author

Landed in 2d35607.

mscdex closed this Oct 13, 2015

bergus commented Oct 13, 2015

Copy link
Copy Markdown

Thanks for the quick fix!

rvagg mentioned this pull request Oct 21, 2015
Merged

MylesBorins commented Oct 23, 2015

Copy link
Copy Markdown
Contributor

@jasnell should this be LTS?

jasnell commented Oct 24, 2015

Copy link
Copy Markdown
Member

hmmm... we may need an LTS WG discussion on this one.

jasnell commented Oct 26, 2015

Copy link
Copy Markdown
Member

@nodejs/lts ... should we land this one in v4.x?

rvagg mentioned this pull request Oct 28, 2015
Closed

rvagg commented Oct 28, 2015

Copy link
Copy Markdown
Member

after looking at #3284 I would vote yes for this one, if this were just a "make it proper" fix then I'd be borderline but given this is fixing an actual user-reported bug it's much easier to justify because it's not academic

srl295 added the i18n-api Issues and PRs related to the i18n implementation. label Oct 28, 2015
mscdex added a commit that referenced this pull request Oct 28, 2015
src: fix exception message encoding on Windows
9bffceb 
The printf family of functions do not properly display UTF8
strings well on Windows. Use the appropriate wide character
API instead if stderr is a tty.

PR-URL: #3288
Fixes: #3284
Reviewed-By: Bert Belder <bertbelder@gmail.com>

jasnell commented Oct 28, 2015

Copy link
Copy Markdown
Member

Landed in v4.x-staging in 9bffceb

mscdex added a commit that referenced this pull request Oct 29, 2015
src: fix exception message encoding on Windows
fe01d0d 
The printf family of functions do not properly display UTF8
strings well on Windows. Use the appropriate wide character
API instead if stderr is a tty.

PR-URL: #3288
Fixes: #3284
Reviewed-By: Bert Belder <bertbelder@gmail.com>
jasnell mentioned this pull request Oct 29, 2015
Closed
mscdex deleted the fix-exception-encoding branch December 17, 2015 18:22
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. i18n-api Issues and PRs related to the i18n implementation. windows Issues and PRs related to the Windows platform.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

Back | FazBrowse Home | New Git URL