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

src: enhance C++ sprintf utility by himself65 · Pull Request #32385 · nodejs/node · GitHub

/ node Public

src: enhance C++ sprintf utility - #32385

Closed
himself65 wants to merge 1 commit into
nodejs:masterfrom
himself65:20200320-debug
ConversationCommits (1)ChecksFiles changed
Closed

src: enhance C++ sprintf utility#32385
himself65 wants to merge 1 commit into
nodejs:masterfrom
himself65:20200320-debug

Conversation

himself65 commented Mar 20, 2020

Copy link
Copy Markdown
Member

fix #32370

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Mar 20, 2020
himself65 changed the title [WIP]src: enhance debug string format [WIP]src: enhance C++ sprintf utility Mar 20, 2020
himself65 force-pushed the 20200320-debug branch 2 times, most recently from 43287bc to 658075e Compare March 20, 2020 08:32
himself65 changed the title [WIP]src: enhance C++ sprintf utility src: enhance C++ sprintf utility Mar 20, 2020
himself65 force-pushed the 20200320-debug branch 4 times, most recently from ddba87b to f363ae5 Compare March 20, 2020 09:23
addaleax reviewed Mar 20, 2020
View reviewed changes
Comment thread src/debug_utils-inl.h Outdated

addaleax Mar 20, 2020

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 don’t think you need to do any templating for T, any value that’s passed to a function like this will be convertible to unsigned long long anyway.

himself65 Mar 20, 2020 •
edited
Loading

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

Of course ok, but we had code more template for other types if we do this.

btw, I inspired from this https://github.com/fmtlib/fmt/blob/5d32ccfc3130fdd122c344b37087ac7fd8c2b62e/include/fmt/format.h#L901-L913

bnoordhuis reviewed Mar 20, 2020
View reviewed changes
Comment thread src/debug_utils-inl.h Outdated
Comment thread src/debug_utils-inl.h Outdated
addaleax approved these changes Mar 20, 2020
View reviewed changes

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

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

Looks good, although it’s probably much simpler to go the %p route and just let the system snprintf do the work.

gabrielschulhof reviewed Mar 20, 2020
View reviewed changes
Comment thread src/debug_utils-inl.h Outdated

himself65 commented Mar 21, 2020

Copy link
Copy Markdown
Member Author

Travis-ci fail because of timeout

https://travis-ci.com/github/nodejs/node/jobs/300545913#L3154-L3159

gabrielschulhof commented Mar 21, 2020

Copy link
Copy Markdown
Contributor

@himself65 I restarted the Travis job.

nodejs-github-bot commented Mar 25, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/30042/

nodejs-github-bot commented Mar 25, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/30043/

nodejs-github-bot commented Mar 25, 2020 •
edited by gabrielschulhof
Loading

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/30075/ ✅

gabrielschulhof pushed a commit that referenced this pull request Mar 26, 2020
src: enhance C++ sprintf utility
dade90d 
PR-URL: #32385
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>

gabrielschulhof commented Mar 26, 2020

Copy link
Copy Markdown
Contributor

Landed in dade90d.

himself65 deleted the 20200320-debug branch March 26, 2020 02:48
MylesBorins pushed a commit that referenced this pull request Mar 26, 2020
src: enhance C++ sprintf utility
94952b4 
PR-URL: #32385
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
MylesBorins mentioned this pull request Mar 26, 2020
Merged
addaleax mentioned this pull request Mar 29, 2020
Closed
2 tasks
himself65 mentioned this pull request Mar 30, 2020
Closed
2 tasks
targos pushed a commit that referenced this pull request Apr 22, 2020
src: enhance C++ sprintf utility
4b5fd24 
PR-URL: #32385
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
targos mentioned this pull request Apr 22, 2020
Merged
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

bnoordhuis bnoordhuis left review comments
addaleax addaleax approved these changes
jasnell jasnell approved these changes
gabrielschulhof gabrielschulhof approved these changes

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

node::Debug does not understand %x

6 participants

Back | FazBrowse Home | New Git URL