GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — tools: fix compiler warning in inspector_protocol by RaisinTen · Pull Request #37573 · nodejs/node

Conversation

@RaisinTen

@RaisinTen 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_) {
      |                               ~~~~~~~~~~~~~~~~~~~~~~~~~~~

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss and fernando-mf reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Mar 2, 2021
@RaisinTen RaisinTen mentioned this pull request Mar 2, 2021
v15.11.0 release proposal #37569
Merged
@aduh95

aduh95 commented Mar 2, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@RaisinTen

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@RaisinTen

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@richardlau

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 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@RaisinTen
RaisinTen force-pushed the tools/fix-compiler-warning-in-encoding.cc branch from c66c751 to 12351ca Compare March 3, 2021 11:35
@RaisinTen RaisinTen changed the title tools: fix compiler warning in encoding.cc tools: fix compiler warning in inspector_protocol Mar 3, 2021
@RaisinTen

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@RaisinTen

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 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
aduh95
aduh95 approved these changes Mar 3, 2021
View reviewed changes

@aduh95 aduh95 left a comment

Copy link
Copy Markdown
Contributor

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.

00e4866 LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
RaisinTen reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@nodejs-github-bot

nodejs-github-bot commented Mar 4, 2021

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aduh95 aduh95 added the inspector Issues and PRs related to the V8 inspector protocol. label Mar 4, 2021
@aduh95

aduh95 commented Mar 4, 2021

Copy link
Copy Markdown
Contributor

ping @nodejs/inspector for reviews
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
benjamingr
benjamingr approved these changes Mar 4, 2021
View reviewed changes
@nodejs-github-bot

nodejs-github-bot commented Mar 4, 2021

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Mar 4, 2021

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Mar 4, 2021

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@RaisinTen @aduh95
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 <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@aduh95
aduh95 force-pushed the tools/fix-compiler-warning-in-encoding.cc branch from 12351ca to ffb34b6 Compare March 4, 2021 21:27
@aduh95

aduh95 commented Mar 4, 2021

Copy link
Copy Markdown
Contributor

Landed in ffb34b6
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aduh95
aduh95 merged commit ffb34b6 into nodejs:master Mar 4, 2021
@RaisinTen
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
@RaisinTen @danielleadams
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 <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@danielleadams danielleadams mentioned this pull request Mar 16, 2021
v15.12.0 proposal #37766
Merged
targos pushed a commit that referenced this pull request May 1, 2021
@RaisinTen @targos
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 <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@danielleadams danielleadams mentioned this pull request May 3, 2021
v14.17.0 proposal #38507
Merged
@Trott

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 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@RaisinTen

RaisinTen commented Aug 7, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

Upstream PR: https://chromium-review.googlesource.com/c/deps/inspector_protocol/+/3077907 (🥳 merged!)
Trott reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
richardlau pushed a commit to richardlau/node-1 that referenced this pull request Aug 10, 2021
@RaisinTen @richardlau
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 <[email protected]>
    Reviewed-By: Benjamin Gruenbaum <[email protected]>
jasnell pushed a commit that referenced this pull request Aug 12, 2021
@RaisinTen @jasnell
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 <[email protected]>
    Reviewed-By: Benjamin Gruenbaum <[email protected]>

PR-URL: #39725
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
danielleadams pushed a commit that referenced this pull request Aug 16, 2021
@RaisinTen @danielleadams
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 <[email protected]>
    Reviewed-By: Benjamin Gruenbaum <[email protected]>

PR-URL: #39725
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Sep 4, 2021
@RaisinTen @targos
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 <[email protected]>
    Reviewed-By: Benjamin Gruenbaum <[email protected]>

PR-URL: #39725
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benjamingr benjamingr benjamingr approved these changes
@aduh95 aduh95 aduh95 approved these changes

Uh oh!

There was an error while loading. Please reload this page.

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.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@RaisinTen @aduh95 @richardlau @nodejs-github-bot @Trott @benjamingr
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.