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

src: fix compiler warnings by cjihrig · Pull Request #25165 · nodejs/node · GitHub

/ node Public

src: fix compiler warnings - #25165

Closed
cjihrig wants to merge 1 commit into
nodejs:masterfrom
cjihrig:warning
ConversationCommits (1)ChecksFiles changed
Closed

src: fix compiler warnings#25165
cjihrig wants to merge 1 commit into
nodejs:masterfrom
cjihrig:warning

Conversation

cjihrig commented Dec 21, 2018

Copy link
Copy Markdown
Contributor

The warnings in question are:

../src/node.cc:844:13: warning: unused function 'DebugProcess' [-Wunused-function]
static void DebugProcess(const FunctionCallbackInfo<Value>& args);
            ^
../src/node.cc:845:13: warning: unused function 'DebugEnd' [-Wunused-function]
static void DebugEnd(const FunctionCallbackInfo<Value>& args);
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

src: fix compiler warnings
da4adad 
The warnings in question are:

../src/node.cc:844:13: warning: unused function 'DebugProcess' [-Wunused-function]
static void DebugProcess(const FunctionCallbackInfo<Value>& args);
            ^
../src/node.cc:845:13: warning: unused function 'DebugEnd' [-Wunused-function]
static void DebugEnd(const FunctionCallbackInfo<Value>& args);

nodejs-github-bot commented Dec 21, 2018

Copy link
Copy Markdown
Collaborator

@cjihrig build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2025/pipeline

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Dec 21, 2018
richardlau approved these changes Dec 22, 2018
View reviewed changes

richardlau left a comment

Copy link
Copy Markdown
Member

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

In case anyone else was wondering, the functions being referred to were moved in #25127.

Trott commented Dec 23, 2018

Copy link
Copy Markdown
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/19766/ ✔️

Trott added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Dec 23, 2018

Trott commented Dec 23, 2018

Copy link
Copy Markdown
Member

The quoted error messages in the commit message are longer than 72 characters. What do we normally do in this case? Land it as-is? Wrap the error message even though that's altering quoted output? Move the error messages to a gist or something and link there?

cjihrig commented Dec 24, 2018

Copy link
Copy Markdown
Contributor Author

I'm not sure if we have a general policy on that. In this particular case, [-Wunused-function] is not important and can be dropped.

danbev commented Dec 24, 2018

Copy link
Copy Markdown
Contributor

The quoted error messages in the commit message are longer than 72 characters. What do we normally do in this case? Land it as-is?

I usually line break it and try to make sure that the ^ still lines up:

../src/node.cc:844:13: warning:
unused function 'DebugProcess' [-Wunused-function]
static void DebugProcess(const FunctionCallbackInfo<Value>& args);
                ^
../src/node.cc:845:13: warning:
unused function 'DebugEnd' [-Wunused-function]
static void DebugEnd(const FunctionCallbackInfo<Value>& args);

I'll do that for this as well but let me know if I should be doing something different in the future.

danbev commented Dec 24, 2018

Copy link
Copy Markdown
Contributor

Landed in e7e26b2. 🎄

danbev closed this Dec 24, 2018
cjihrig deleted the warning branch December 24, 2018 15:43
MylesBorins referenced this pull request Dec 25, 2018
src: fix compiler warnings
e7e26b2 
The warnings in question are:

../src/node.cc:844:13: warning:
unused function 'DebugProcess' [-Wunused-function]
static void DebugProcess(const FunctionCallbackInfo<Value>& args);
            ^
../src/node.cc:845:13: warning:
unused function 'DebugEnd' [-Wunused-function]
static void DebugEnd(const FunctionCallbackInfo<Value>& args);

MylesBorins commented Dec 25, 2018

Copy link
Copy Markdown
Contributor

landing this on v11.x causes there to be an error as the symbol is still being used. Unfortunately as the commit meta data was not included in the commit when it landed on master there will be no way for our current tooling to ignore this commit when auditing master for v11.x

We likely want to revisit this and try and come up with a solution so it doesn't keep showing up as a false positive when auditing

danbev commented Dec 25, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

@MylesBorins I really sorry about this 😞 Is there anything I can do to fix this or is it too late?
Would it help if I reverted this and commit with the metadata?

MylesBorins commented Dec 25, 2018 via email

Copy link
Copy Markdown
Contributor

addaleax pushed a commit to addaleax/node that referenced this pull request Jan 14, 2019
src: fix compiler warnings
5b780eb 
The warnings in question are:

../src/node.cc:844:13: warning:
unused function 'DebugProcess' [-Wunused-function]
static void DebugProcess(const FunctionCallbackInfo<Value>& args);
            ^
../src/node.cc:845:13: warning:
unused function 'DebugEnd' [-Wunused-function]
static void DebugEnd(const FunctionCallbackInfo<Value>& args);

PR-URL: nodejs#25165
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
addaleax mentioned this pull request Jan 14, 2019
Closed
addaleax pushed a commit that referenced this pull request Jan 15, 2019
src: fix compiler warnings
f8ba488 
The warnings in question are:

../src/node.cc:844:13: warning:
unused function 'DebugProcess' [-Wunused-function]
static void DebugProcess(const FunctionCallbackInfo<Value>& args);
            ^
../src/node.cc:845:13: warning:
unused function 'DebugEnd' [-Wunused-function]
static void DebugEnd(const FunctionCallbackInfo<Value>& args);

PR-URL: #25165
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>

Backport-PR-URL: #25496
BridgeAR mentioned this pull request Jan 16, 2019
Merged
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
src: fix compiler warnings
4e702e5 
The warnings in question are:

../src/node.cc:844:13: warning:
unused function 'DebugProcess' [-Wunused-function]
static void DebugProcess(const FunctionCallbackInfo<Value>& args);
            ^
../src/node.cc:845:13: warning:
unused function 'DebugEnd' [-Wunused-function]
static void DebugEnd(const FunctionCallbackInfo<Value>& args);

PR-URL: nodejs#25165
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>

Backport-PR-URL: nodejs#25496
MylesBorins mentioned this pull request Jan 24, 2019
Merged
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

addaleax addaleax approved these changes
lpinca lpinca approved these changes
richardlau richardlau approved these changes
+1 more reviewer
danbev danbev approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

Back | FazBrowse Home | New Git URL