Skip to content

src: enhance C++ sprintf utility - #32385

Closed
himself65 wants to merge 1 commit into
nodejs:masterfrom
himself65:20200320-debug
Closed

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

Conversation

@himself65

@himself65himself65 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-botnodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Mar 20, 2020
@himself65himself65 changed the title [WIP]src: enhance debug string format[WIP]src: enhance C++ sprintf utilityMar 20, 2020
@himself65
himself65force-pushed the 20200320-debug branch 2 times, most recently from 43287bc to 658075eCompareMarch 20, 2020 08:32
@himself65himself65 changed the title [WIP]src: enhance C++ sprintf utilitysrc: enhance C++ sprintf utilityMar 20, 2020
@himself65
himself65force-pushed the 20200320-debug branch 4 times, most recently from ddba87b to f363ae5CompareMarch 20, 2020 09:23
addaleax
addaleax reviewed Mar 20, 2020
View reviewed changes
Comment threadsrc/debug_utils-inl.h Outdated

@addaleaxaddaleaxMar 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.

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.

@himself65himself65Mar 20, 2020•
edited
Loading

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.

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
bnoordhuis reviewed Mar 20, 2020
View reviewed changes
Comment threadsrc/debug_utils-inl.h Outdated
Comment threadsrc/debug_utils-inl.h Outdated
addaleax
addaleax approved these changes Mar 20, 2020
View reviewed changes

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

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

gabrielschulhof
gabrielschulhof reviewed Mar 20, 2020
View reviewed changes
Comment threadsrc/debug_utils-inl.h Outdated
@himself65

himself65 commented Mar 21, 2020

Copy link
Copy Markdown
MemberAuthor

Travis-ci fail because of timeout

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

@gabrielschulhof

gabrielschulhof commented Mar 21, 2020

Copy link
Copy Markdown
Contributor

@himself65 I restarted the Travis job.

@nodejs-github-bot

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

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

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
@himself65Gabriel Schulhof
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

gabrielschulhof commented Mar 26, 2020

Copy link
Copy Markdown
Contributor

Landed in dade90d.

@himself65
himself65 deleted the 20200320-debug branch March 26, 2020 02:48
MylesBorins pushed a commit that referenced this pull request Mar 26, 2020
@himself65@MylesBorins
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>
@MylesBorinsMylesBorins mentioned this pull request Mar 26, 2020
Merged
@addaleaxaddaleax mentioned this pull request Mar 29, 2020
Closed
2 tasks
@himself65himself65 mentioned this pull request Mar 30, 2020
Closed
2 tasks
targos pushed a commit that referenced this pull request Apr 22, 2020
@himself65@targos
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>
@targostargos mentioned this pull request Apr 22, 2020
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuisbnoordhuisbnoordhuis left review comments
@addaleaxaddaleaxaddaleax approved these changes
@jasnelljasnelljasnell approved these changes
@gabrielschulhofgabrielschulhofgabrielschulhof 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

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