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

tools: fix compiler warning in inspector_protocol by RaisinTen · Pull Request #37573 · nodejs/node · GitHub

/ node Public

tools: fix compiler warning in inspector_protocol - #37573

Merged
aduh95 merged 1 commit into
nodejs:masterfrom
RaisinTen:tools/fix-compiler-warning-in-encoding.cc
Mar 4, 2021
ConversationCommits (1)ChecksFiles changed
Merged

tools: fix compiler warning in inspector_protocol#37573
aduh95 merged 1 commit into
nodejs:masterfrom
RaisinTen:tools/fix-compiler-warning-in-encoding.cc

Conversation

RaisinTen commented Mar 2, 2021

Copy link
Copy Markdown
Member 
error: comparison of integer expressions of different signedness:
‘int’ and ‘uint64_t’ {aka ‘long unsigned int’} [-Werror=sign-compare]
 2562 |           if (!success || std::numeric_limits<int32_t>::max() <
      |                           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
 2563 |                               token_start_internal_value_) {
      |                               ~~~~~~~~~~~~~~~~~~~~~~~~~~~

nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Mar 2, 2021
RaisinTen mentioned this pull request Mar 2, 2021
Merged

aduh95 commented Mar 2, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

inspector_protocol is a third party, I'm not sure what is our policy towards adding patches (I know for V8 we require the patch to land upstream first, I don't know if it's a requirement for every third party deps).

EDIT: Actually, there is a precedent: 2e441e1

RaisinTen commented Mar 2, 2021

Copy link
Copy Markdown
Member Author

inspector_protocol is a third party, I'm not sure what is our policy towards adding patches (I know for V8 we require the patch to land upstream first, I don't know if it's a requirement for every third party deps).

EDIT: Actually, there is a precedent: 2e441e1

@aduh95 Sorry, I didn't know that it was a third party project.
The code looks different from the code in the upstream.
I wonder why the changes aren't sent upstream though.

RaisinTen commented Mar 2, 2021

Copy link
Copy Markdown
Member Author

Why isn't coverage-linux / coverage-linux (pull_request) automatically being run for this PR?

richardlau commented Mar 2, 2021

Copy link
Copy Markdown
Member

Why isn't coverage-linux / coverage-linux (pull_request) automatically being run for this PR?

Because it only changes paths that are being ignored 😞

node/.github/workflows/coverage-linux.yml

Lines 5 to 9 in 993963e

paths-ignore:
- 'doc/**'
- 'deps/**'
- 'benchmark/**'
- 'tools/**'

RaisinTen force-pushed the tools/fix-compiler-warning-in-encoding.cc branch from c66c751 to 12351ca Compare March 3, 2021 11:35
RaisinTen changed the title tools: fix compiler warning in encoding.cc tools: fix compiler warning in inspector_protocol Mar 3, 2021

RaisinTen commented Mar 3, 2021

Copy link
Copy Markdown
Member Author

The file probably gets generated during the coverage run here:

node/tools/inspector_protocol/code_generator.py

Line 643 in f6b1df2

"encoding_cpp.template",

Maybe that's why my initial commit had 0 effect.

RaisinTen commented Mar 3, 2021

Copy link
Copy Markdown
Member Author

The warning seems to be gone:

Should I keep the change in tools/inspector_protocol/encoding/encoding.cc?

aduh95 approved these changes Mar 3, 2021
View reviewed changes

aduh95 left a comment

Copy link
Copy Markdown
Contributor

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

00e4866 LGTM

nodejs-github-bot commented Mar 4, 2021

Copy link
Copy Markdown
Collaborator

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

aduh95 added the inspector Issues and PRs related to the V8 inspector protocol label Mar 4, 2021

aduh95 commented Mar 4, 2021

Copy link
Copy Markdown
Contributor

ping @nodejs/inspector for reviews

nodejs-github-bot commented Mar 4, 2021

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Mar 4, 2021

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Mar 4, 2021

Copy link
Copy Markdown
Collaborator

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

tools: fix compiler warning in inspector_protocol
ffb34b6 
error: comparison of integer expressions of different signedness:
‘int’ and ‘uint64_t’ {aka ‘long unsigned int’} [-Werror=sign-compare]
 2562 |           if (!success || std::numeric_limits<int32_t>::max() <
      |                           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
 2563 |                               token_start_internal_value_) {
      |                               ~~~~~~~~~~~~~~~~~~~~~~~~~~~

PR-URL: nodejs#37573
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
aduh95 force-pushed the tools/fix-compiler-warning-in-encoding.cc branch from 12351ca to ffb34b6 Compare March 4, 2021 21:27

aduh95 commented Mar 4, 2021

Copy link
Copy Markdown
Contributor

Landed in ffb34b6

aduh95 merged commit ffb34b6 into nodejs:master Mar 4, 2021
RaisinTen deleted the tools/fix-compiler-warning-in-encoding.cc branch March 5, 2021 13:17
danielleadams pushed a commit that referenced this pull request Mar 16, 2021
tools: fix compiler warning in inspector_protocol
82e78f7 
error: comparison of integer expressions of different signedness:
‘int’ and ‘uint64_t’ {aka ‘long unsigned int’} [-Werror=sign-compare]
 2562 |           if (!success || std::numeric_limits<int32_t>::max() <
      |                           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
 2563 |                               token_start_internal_value_) {
      |                               ~~~~~~~~~~~~~~~~~~~~~~~~~~~

PR-URL: #37573
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
danielleadams mentioned this pull request Mar 16, 2021
Merged
targos pushed a commit that referenced this pull request May 1, 2021
tools: fix compiler warning in inspector_protocol
77eb45a 
error: comparison of integer expressions of different signedness:
‘int’ and ‘uint64_t’ {aka ‘long unsigned int’} [-Werror=sign-compare]
 2562 |           if (!success || std::numeric_limits<int32_t>::max() <
      |                           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
 2563 |                               token_start_internal_value_) {
      |                               ~~~~~~~~~~~~~~~~~~~~~~~~~~~

PR-URL: #37573
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
danielleadams mentioned this pull request May 3, 2021
Merged

Trott commented Aug 7, 2021

Copy link
Copy Markdown
Member

For an upcoming PR, I have to revert this to apply patches for upstream changes. I know there's precedence for updating this dependency directly, but I think we should avoid it going forward and upstream things.

Aside: In the original PR where this was being added, it was initially in deps, but a collaborator asked that it get moved to tools. They're long inactive on the project and I'm reluctant to ping them about it after so long, but I do think deps would have made it a bit more clear that we (probably) shouldn't modify it directly. (That's not necessarily to say that tools is wrong, just that there are tradeoffs.)

RaisinTen commented Aug 7, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

Upstream PR: https://chromium-review.googlesource.com/c/deps/inspector_protocol/+/3077907 (🥳 merged!)

richardlau pushed a commit to richardlau/node-1 that referenced this pull request Aug 10, 2021
tools: cherry-pick ffb34b6
1c2c34a 
Original commit message:

    tools: fix compiler warning in inspector_protocol

    error: comparison of integer expressions of different signedness:
    ‘int’ and ‘uint64_t’ {aka ‘long unsigned int’} [-Werror=sign-compare]
     2562 |           if (!success || std::numeric_limits<int32_t>::max() <
          |                           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
     2563 |                               token_start_internal_value_) {
          |                               ~~~~~~~~~~~~~~~~~~~~~~~~~~~

    PR-URL: nodejs#37573
    Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
    Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
jasnell pushed a commit that referenced this pull request Aug 12, 2021
tools: cherry-pick ffb34b6
a48b9c3 
Original commit message:

    tools: fix compiler warning in inspector_protocol

    error: comparison of integer expressions of different signedness:
    ‘int’ and ‘uint64_t’ {aka ‘long unsigned int’} [-Werror=sign-compare]
     2562 |           if (!success || std::numeric_limits<int32_t>::max() <
          |                           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
     2563 |                               token_start_internal_value_) {
          |                               ~~~~~~~~~~~~~~~~~~~~~~~~~~~

    PR-URL: #37573
    Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
    Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

PR-URL: #39725
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Aug 16, 2021
tools: cherry-pick ffb34b6
77f9c1f 
Original commit message:

    tools: fix compiler warning in inspector_protocol

    error: comparison of integer expressions of different signedness:
    ‘int’ and ‘uint64_t’ {aka ‘long unsigned int’} [-Werror=sign-compare]
     2562 |           if (!success || std::numeric_limits<int32_t>::max() <
          |                           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
     2563 |                               token_start_internal_value_) {
          |                               ~~~~~~~~~~~~~~~~~~~~~~~~~~~

    PR-URL: #37573
    Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
    Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

PR-URL: #39725
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Sep 4, 2021
tools: cherry-pick ffb34b6
09630cf 
Original commit message:

    tools: fix compiler warning in inspector_protocol

    error: comparison of integer expressions of different signedness:
    ‘int’ and ‘uint64_t’ {aka ‘long unsigned int’} [-Werror=sign-compare]
     2562 |           if (!success || std::numeric_limits<int32_t>::max() <
          |                           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
     2563 |                               token_start_internal_value_) {
          |                               ~~~~~~~~~~~~~~~~~~~~~~~~~~~

    PR-URL: #37573
    Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
    Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

PR-URL: #39725
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
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

benjamingr benjamingr approved these changes
aduh95 aduh95 approved these changes

Assignees

No one assigned

Labels

inspector Issues and PRs related to the V8 inspector protocol tools Issues and PRs related to the tools directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL