Skip to content

tools: fix compiler warning in inspector_protocol#37573

Merged
aduh95 merged 1 commit intonodejs:masterfrom
RaisinTen:tools/fix-compiler-warning-in-encoding.cc
Mar 4, 2021
Merged

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

Conversation

@RaisinTen
Copy link
Member

@RaisinTen RaisinTen commented Mar 2, 2021 

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 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
Merged
@aduh95
Copy link
Contributor

aduh95 commented Mar 2, 2021 •
edited
Loading

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
Copy link
Member Author

RaisinTen commented Mar 2, 2021

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
Copy link
Member Author

RaisinTen commented Mar 2, 2021

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

@richardlau
Copy link
Member

richardlau commented Mar 2, 2021

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 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
Copy link
Member Author

RaisinTen commented Mar 3, 2021

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
Copy link
Member Author

RaisinTen commented Mar 3, 2021

The warning seems to be gone:

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

aduh95
aduh95 approved these changes Mar 3, 2021
View reviewed changes
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

00e4866 LGTM

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 4, 2021

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

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

aduh95 commented Mar 4, 2021

ping @nodejs/inspector for reviews

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 4, 2021

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 4, 2021

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 4, 2021

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

@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 <duhamelantoine1995@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@aduh95 aduh95 force-pushed the tools/fix-compiler-warning-in-encoding.cc branch from 12351ca to ffb34b6 Compare March 4, 2021 21:27
@aduh95
Copy link
Contributor

aduh95 commented Mar 4, 2021

Landed in ffb34b6

@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 <duhamelantoine1995@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@danielleadams danielleadams mentioned this pull request Mar 16, 2021
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 <duhamelantoine1995@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@danielleadams danielleadams mentioned this pull request May 3, 2021
Merged
@Trott
Copy link
Member

Trott commented Aug 7, 2021

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
Copy link
Member Author

RaisinTen commented Aug 7, 2021 •
edited
Loading

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
@RaisinTen @richardlau
tools: cherry-pick fb34b6d5dbf0
d5d714a 
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>
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 <duhamelantoine1995@gmail.com>
    Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
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 <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
@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 <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
@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 <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>
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

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

@RaisinTen @aduh95 @richardlau @nodejs-github-bot @Trott @benjamingr