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

Use cares_get_32bit as a function helper #34944

Closed
wants to merge 1 commit into from

Conversation

juanarbol
Copy link
Member

@juanarbol juanarbol commented Aug 27, 2020

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@juanarbol juanarbol requested a review from a team as a code owner August 27, 2020 01:57
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added 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. labels Aug 27, 2020
src/base64.h Outdated Show resolved Hide resolved
src/base64.h Outdated Show resolved Hide resolved
src/util-inl.h Outdated Show resolved Hide resolved
@juanarbol
Copy link
Member Author

Refs: #34827

@juanarbol juanarbol added wip Issues and PRs that are still a work in progress. and removed wip Issues and PRs that are still a work in progress. labels Aug 27, 2020
src/util.h Outdated
@@ -326,6 +326,8 @@ inline bool StringEqualNoCase(const char* a, const char* b);
// strncasecmp() is locale-sensitive. Use StringEqualNoCaseN() instead.
inline bool StringEqualNoCaseN(const char* a, const char* b, size_t length);

inline uint32_t ReadUint32BE(const unsigned char* p);
Copy link
Member

Choose a reason for hiding this comment

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

This should go into base64.h if the corresponding function is defined in base64-inl.h, I would say

src/util-inl.h Outdated
@@ -27,6 +27,7 @@
#include <cmath>
#include <cstring>
#include "util.h"
#include "base64-inl.h"
Copy link
Member

Choose a reason for hiding this comment

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

It’s not common practice to include another header just so that its definitions are also available from here – it’s probably better to remove this line and add it to the source files that use the ReadUint32BE function?

@juanarbol
Copy link
Member Author

It is compiling with clang++

 ccache /usr/bin//clang++ -o /home/juanarbol/GitHub/node/out/Release/node -pthread -rdynamic -m64 /home/juanarbol/GitHub/node/out/Release/obj.target/node_text_start/src/large_pages/node_text_start.o -Wl,--whole-archive /home/juanarbol/GitHub/node/out/Release/obj.target/libnode.a /home/juanarbol/GitHub/node/out/Release/obj.target/tools/v8_gypfiles/libv8_base_without_compiler.a -Wl,--no-whole-archive -Wl,--whole-archive /home/juanarbol/GitHub/node/out/Release/obj.target/deps/zlib/libzlib.a -Wl,--no-whole-archive -Wl,--whole-archive /home/juanarbol/GitHub/node/out/Release/obj.target/deps/uv/libuv.a -Wl,--no-whole-archive -Wl,-z,noexecstack -Wl,--whole-archive /home/juanarbol/GitHub/node/out/Release/obj.target/tools/v8_gypfiles/libv8_snapshot.a -Wl,--no-whole-archive -Wl,-z,relro -Wl,-z,now -Wl,--whole-archive,/home/juanarbol/GitHub/node/out/Release/obj.target/deps/openssl/libopenssl.a -Wl,--no-whole-archive -pthread  -Wl,--start-group /home/juanarbol/GitHub/node/out/Release/obj.target/node/src/node_main.o /home/juanarbol/GitHub/node/out/Release/obj.target/node/gen/node_code_cache.o /home/juanarbol/GitHub/node/out/Release/obj.target/node/gen/node_snapshot.o /home/juanarbol/GitHub/node/out/Release/obj.target/deps/histogram/libhistogram.a /home/juanarbol/GitHub/node/out/Release/obj.target/deps/uvwasi/libuvwasi.a /home/juanarbol/GitHub/node/out/Release/obj.target/libnode.a /home/juanarbol/GitHub/node/out/Release/obj.target/libnode_text_start.a /home/juanarbol/GitHub/node/out/Release/obj.target/tools/v8_gypfiles/libv8_snapshot.a /home/juanarbol/GitHub/node/out/Release/obj.target/tools/v8_gypfiles/libv8_libplatform.a /home/juanarbol/GitHub/node/out/Release/obj.target/tools/icu/libicui18n.a /home/juanarbol/GitHub/node/out/Release/obj.target/deps/zlib/libzlib.a /home/juanarbol/GitHub/node/out/Release/obj.target/deps/llhttp/libllhttp.a /home/juanarbol/GitHub/node/out/Release/obj.target/deps/cares/libcares.a /home/juanarbol/GitHub/node/out/Release/obj.target/deps/uv/libuv.a /home/juanarbol/GitHub/node/out/Release/obj.target/deps/nghttp2/libnghttp2.a /home/juanarbol/GitHub/node/out/Release/obj.target/deps/brotli/libbrotli.a /home/juanarbol/GitHub/node/out/Release/obj.target/deps/openssl/libopenssl.a /home/juanarbol/GitHub/node/out/Release/obj.target/tools/icu/libicuucx.a /home/juanarbol/GitHub/node/out/Release/obj.target/tools/icu/libicudata.a /home/juanarbol/GitHub/node/out/Release/obj.target/tools/v8_gypfiles/libv8_base_without_compiler.a /home/juanarbol/GitHub/node/out/Release/obj.target/tools/v8_gypfiles/libv8_libbase.a /home/juanarbol/GitHub/node/out/Release/obj.target/tools/v8_gypfiles/libv8_libsampler.a /home/juanarbol/GitHub/node/out/Release/obj.target/tools/v8_gypfiles/libv8_zlib.a /home/juanarbol/GitHub/node/out/Release/obj.target/tools/v8_gypfiles/libv8_compiler.a /home/juanarbol/GitHub/node/out/Release/obj.target/tools/v8_gypfiles/libv8_initializers.a -latomic -lm -ldl -Wl,--end-group
rm fc49da6c53794ce8643a014f0dae2e757fbf8b5d.intermediate
if [ ! -r node -o ! -L node ]; then \
  ln -fs out/Release/node node; fi

@@ -4,6 +4,7 @@
#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS

#include "util.h"
#include "base64-inl.h"
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove this line, and replace #include "base64.h" with #include "base64-inl.h" instead where necessary?

The general idea here is:

  • .h header files contain declarations, and sometimes some definitions that don’t require including other headers (e.g. for getters/setters)
    • .h header files include only other .h headers
  • -inl.h header files contain definitions of inline functions from the corresponding .h header file, i.e. functions marked inline in the declaration or template functions
    • -inl.h header files include the corresponding .h header file, and other .h and -inl.h header files as needed
    • It’s not always necessary to split out the definitions from the .h file into an -inl.h file, but it becomes necessary when the contents of other -inl.h files start being used, and it is very much recommended when the functions are longer than a few lines to keep the corresponding .h file readable
    • All visible definitions from this file should have been declared in the corresponding .h header file
  • .cc files contain definitions of non-inline functions from the corresponding .h header file
    • .cc files also include the corresponding .h header file, and other .h and -inl.h header files as needed

I hope this is helpful?

lundibundi added a commit to lundibundi/node that referenced this pull request Aug 31, 2020
Refs: nodejs#34944 (comment)
Co-authored-by: Anna Henningsen <anna@addaleax.net>
@juanarbol
Copy link
Member Author

Hey, sorry about the delay. :/

@juanarbol
Copy link
Member Author

Ping @addaleax

you think is ok to land this one?

@addaleax
Copy link
Member

@juanarbol I think this comment is outstanding: #34944 (comment)

If you prefer, you can merge this and I’ll open a PR to address that comment, that’s also totally fine 👍 (This needs another CI though, so I’ll kick one off.)

@addaleax addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 29, 2020
@juanarbol
Copy link
Member Author

juanarbol commented Sep 29, 2020

@addaleax, Shame on me, haha, the comment is totally clear, but I don't get it (I'm kind of a non-C++ programmer), so when I try to follow that out, I just get a compile or linking error (with no very useful information).

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 29, 2020
@addaleax
Copy link
Member

Landed in ff4cf81 :)

@addaleax addaleax closed this Sep 30, 2020
addaleax pushed a commit that referenced this pull request Sep 30, 2020
Fixes: #34827

PR-URL: #34944
Reviewed-By: Anna Henningsen <anna@addaleax.net>
addaleax added a commit to addaleax/node that referenced this pull request Sep 30, 2020
nodejs-github-bot pushed a commit that referenced this pull request Oct 3, 2020
Refs: #34944 (comment)

PR-URL: #35432
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BethGriggs
Copy link
Member

This had a large number of changes/conflicts on v14.x, please raise a backport PR if you think it should land on v14.x

@juanarbol
Copy link
Member Author

I could work on backporting.

aduh95 pushed a commit that referenced this pull request Nov 9, 2020
Refs: #34944 (comment)
Co-authored-by: Anna Henningsen <anna@addaleax.net>

PR-URL: #35000
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
danielleadams pushed a commit that referenced this pull request Nov 9, 2020
Refs: #34944 (comment)
Co-authored-by: Anna Henningsen <anna@addaleax.net>

PR-URL: #35000
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
BethGriggs pushed a commit that referenced this pull request Dec 9, 2020
Fixes: #34827

Backport-PR-URL: #35701
PR-URL: #34944
Reviewed-By: Anna Henningsen <anna@addaleax.net>
BethGriggs pushed a commit that referenced this pull request Dec 9, 2020
Refs: #34944 (comment)
Co-authored-by: Anna Henningsen <anna@addaleax.net>

PR-URL: #35000
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
Fixes: #34827

Backport-PR-URL: #35701
PR-URL: #34944
Reviewed-By: Anna Henningsen <anna@addaleax.net>
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
Refs: #34944 (comment)
Co-authored-by: Anna Henningsen <anna@addaleax.net>

PR-URL: #35000
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@BethGriggs BethGriggs mentioned this pull request Dec 10, 2020
BethGriggs pushed a commit that referenced this pull request Dec 15, 2020
Fixes: #34827

Backport-PR-URL: #35701
PR-URL: #34944
Reviewed-By: Anna Henningsen <anna@addaleax.net>
BethGriggs pushed a commit that referenced this pull request Dec 15, 2020
Refs: #34944 (comment)
Co-authored-by: Anna Henningsen <anna@addaleax.net>

PR-URL: #35000
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
Fixes: nodejs#34827

PR-URL: nodejs#34944
Reviewed-By: Anna Henningsen <anna@addaleax.net>
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
Refs: nodejs#34944 (comment)

PR-URL: nodejs#35432
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@juanarbol juanarbol deleted the juanarbol/bitwise-helper branch January 19, 2021 16:25
targos pushed a commit that referenced this pull request May 1, 2021
Refs: #34944 (comment)

PR-URL: #35432
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@targos targos removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Sep 5, 2021
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
Development

Successfully merging this pull request may close these issues.

6 participants