Skip to content
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

zlib: alter wrong argument number error message #22130

Closed
wants to merge 1 commit into from

Conversation

amarchino
Copy link

The ZCtx::Init function presents a standard CHECK to handle the
argument number. The correct number of arguments as of now is 7.

The CHECK usage follows the standard usage in the node C++ code, yet
this is the only instance where the error message is long enough so as
to be split in two lines. The macro CHECK displays this error message
incorrectly.

The fix revolves around a copy of the macro, injected in the point of
code where the previous CHECK was residing; it formats the message in
the same way the macro would have and cleanly exits the execution.

Fixes: #16987

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

Notes:

  • Commit message guidelines followed, yet core-validate-commit fails due to absence of PR-URL and reviewer (might it be acceptable in opening the pull request?)
  • I was not able to create a test for the code correction. I'd like to ask for support in how to create a test case.

I remain at disposal for every problem that maight arise.
Thanks!

The ZCtx::Init function presents a standard CHECK to handle the
argument number. The correct number of arguments as of now is 7.

The CHECK usage follows the standard usage in the node C++ code, yet
this is the only instance where the error message is long enough so as
to be split in two lines. The macro CHECK displays this error message
incorrectly.

The fix revolves around a copy of the macro, injected in the point of
code where the previous CHECK was residing; it formats the message in
the same way the macro would have and cleanly exits the execution.

Fixes: nodejs#16987
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. zlib Issues and PRs related to the zlib subsystem. labels Aug 4, 2018
@tniessen
Copy link
Member

tniessen commented Aug 5, 2018

Thank you for your contribution, @amarchino! 😃

I won't stand in the way, but to be honest, I am not sure whether the relatively small improvement in readability of a debugging message that should barely ever be visible to users justifies the added complexity. Plus, there are many other places in core which use the same or similar ways to include custom messages in assertions.

@amarchino
Copy link
Author

I am, for the most part, in agreement with Your point, @tniessen.
This pull request was done as a (long time due) correction for a typo found some time ago.
If the added complexity introduces is not worth the slight improvement, the pull request (and arguably the referenced issue) should be closed with no damage incurred.
I defer to You for the evaluation, and remain at disposal.

Thanks!

@tniessen
Copy link
Member

cc @addaleax based on the discussion in #16987.

@addaleax
Copy link
Member

I don’t think I have anything to add to @tniessen’s reply here. :)

@tniessen
Copy link
Member

Thanks Anna. I am closing this based on the previous discussion. Thank you for your contribution, @amarchino, we are looking forward to more!

@tniessen tniessen closed this Aug 19, 2018
@amarchino
Copy link
Author

Thanks to all af You for the attention and time You spent!

Sign up for free to 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++. zlib Issues and PRs related to the zlib subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error message possible typo in node_zlib
4 participants