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

src: squelch unused function warnings in util.h #9083

Closed
bnoordhuis opened this issue Oct 13, 2016 · 13 comments · May be fixed by aliscco/alisco-node#705, leonardoadame/node#246, aliscco/alisco-node#826 or aliscco/alisco-node#846
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory.

Comments

@bnoordhuis
Copy link
Member

Paper-cut issue. When you include util.h but not util-inl.h, you get the following warnings:

$ echo -e '#include "util.h" \n int main(){}' | \
  g++ -DNODE_WANT_INTERNALS=1 -Ideps/v8/include -Ideps/uv/include -Isrc \
      -Wall -Wextra -Wno-unused-parameter -x c++ -
In file included from <stdin>:1:0:
src/util.h:37:11: warning: inline function 'T* node::Malloc(size_t) [with T = char; size_t = long unsigned int]' used but never defined
 inline T* Malloc(size_t n);
           ^~~~~~
src/util.h:39:11: warning: inline function 'T* node::Calloc(size_t) [with T = char; size_t = long unsigned int]' used but never defined
 inline T* Calloc(size_t n);
           ^~~~~~
src/util.h:28:11: warning: inline function 'T* node::UncheckedMalloc(size_t) [with T = char; size_t = long unsigned int]' used but never defined
 inline T* UncheckedMalloc(size_t n);
           ^~~~~~~~~~~~~~~
src/util.h:30:11: warning: inline function 'T* node::UncheckedCalloc(size_t) [with T = char; size_t = long unsigned int]' used but never defined
 inline T* UncheckedCalloc(size_t n);
           ^~~~~~~~~~~~~~~
@bnoordhuis bnoordhuis added c++ Issues and PRs that require attention from people who are familiar with C++. good first issue Issues that are suitable for first-time contributors. lib / src Issues and PRs related to general changes in the lib or src directory. labels Oct 13, 2016
@tanujasawant
Copy link
Contributor

I think the compiler needs to know the function to actually inline it. So including the implementation in the header file should solve the problem. util-inl.h has the implementation, whereas util.h does not, hence the warnings. Simply copying the implementation to util.h will work

@tanujasawant
Copy link
Contributor

I'll reference a pull request soon

@tanujasawant
Copy link
Contributor

@bnoordhuis, do I need to make changes to util.h to add implementation or was it just a doubt you wanted to clarify? Because adding implementation would be equivalent to using util.h

@bnoordhuis
Copy link
Member Author

I was thinking more of adding a NODE_UNUSED macro that evaluates to __attribute__((unused)) on GCC-compatible compilers (or [[gnu::unused]] or [[maybe_unused]]) and to __declspec(whatever_ms_uses) for Visual Studio.

@vitkarpov
Copy link
Contributor

@Tanuja-Sawant hey, are you going to make a macro? Seems this is a good first contribution I'd like to make, but if you're in progress I will not 'course. Anyway I'd like to see the fix when it's done.

@vitkarpov
Copy link
Contributor

@soleboxy ahaha, seems you're ahead of me :)

@vitkarpov
Copy link
Contributor

vitkarpov commented Oct 16, 2016

@bnoordhuis Hi, Ben. Can you explain for a newbie like me what do you mean by making a macro?

Am I right in assuming it should be like this:

#define NODE_UNUSED(expr) __attribute__((expr))

How is it assumed to be applied in this case and why just not to move inlines like @soleboxy did?

@bnoordhuis
Copy link
Member Author

Never mind what I said, I thought the warnings were for the non-templated versions but reading the warnings again, it complains about the non-templated versions using template instances.

@solebox
Copy link
Contributor

solebox commented Oct 16, 2016

@bnoordhuis check out my changes, i responded to your comment. (and offered another solution, wonder what you think about it)
@vitkarpov - sorry man , seemed so interesting to solve this i just went away and did it :)
this is my first cpp PR too :)

@tanujasawant
Copy link
Contributor

No problem! I look forward to see the solution to this issue =)

@vitkarpov
Copy link
Contributor

👍 :)

@solebox
Copy link
Contributor

solebox commented Oct 17, 2016

no problem im on it 👍
@bnoordhuis can you checkout the comment on my last commit change?
i am waiting for your response
thanks :)

@addaleax addaleax removed the good first issue Issues that are suitable for first-time contributors. label Oct 18, 2016
@solebox
Copy link
Contributor

solebox commented Oct 26, 2016

@bnoordhuis - hey ben , sorry to bother but could you approve my PR please?
#9115

solebox added a commit to solebox/node that referenced this issue Nov 10, 2016
Fixes: nodejs#9083

moved function bodies back

src: squelch unused function warnings in util.h.
Fixes: nodejs#9083

moved back to previouse commit

lets hope this is the correct answer
src: squelch unused function warnings in util.h.
Fixes: nodejs#9083

src: squelch unused function warnings in util.h.
Fixes: nodejs#9083

src: squelch unused function warnings in util.h.
Fixes: nodejs#9083

src: squelch unused function warnings in util.h.
Fixes: nodejs#9083

src: squelch unused function warnings in util.h.
Fixes: nodejs#9083
addaleax pushed a commit that referenced this issue Nov 22, 2016
Fixes: #9083

PR-URL: #9115
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
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++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
5 participants