-
Notifications
You must be signed in to change notification settings - Fork 28.9k
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.
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
http2: remove unsupported %.* specifier #35694
Conversation
The debug sprintf doesn't support %.* specifiers Fixes: nodejs#35688
Review requested:
|
Codecov Report
@@ Coverage Diff @@
## master #35694 +/- ##
=======================================
Coverage 96.40% 96.40%
=======================================
Files 220 220
Lines 73681 73681
=======================================
+ Hits 71031 71032 +1
+ Misses 2650 2649 -1
Continue to review full report at Codecov.
|
Are we sure this is safe? According to the documentation (emphasis mine):
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mscdex Currently, this is always true, yes, and I think changing this would have to be considered a breaking change on the nghttp2 side
Non-blocking naive questions: Can we/should we add a test? A lint rule? An assertion or other check/confirmation of NULL-termination? |
I would not add a test for specific debugging statements.
That would be great, but it might be quite a bit of effort.
I don’t see any issue with adding |
Landed in c5b9b5b...7083425 |
The debug sprintf doesn't support %.* specifiers Fixes: #35688 PR-URL: #35694 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ricky Zhou <0x19951125@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com>
The debug sprintf doesn't support %.* specifiers Fixes: #35688 PR-URL: #35694 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ricky Zhou <0x19951125@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com>
The debug sprintf doesn't support %.* specifiers Fixes: #35688 PR-URL: #35694 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ricky Zhou <0x19951125@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com>
The debug sprintf doesn't support %.* specifiers Fixes: #35688 PR-URL: #35694 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ricky Zhou <0x19951125@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com>
The debug sprintf doesn't support %.* specifiers Fixes: #35688 PR-URL: #35694 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ricky Zhou <0x19951125@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com>
The debug sprintf doesn't support %.* specifiers
Fixes: #35688
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes