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,url: separate some tables out of node_url.cc #38988

Closed
wants to merge 5 commits into from

Conversation

XadillaX
Copy link
Member

The purpose of separating is for readability and maintainability.

The purpose of separating is for readability and maintainability.
@github-actions github-actions 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. needs-ci PRs that need a full CI run. labels Jun 10, 2021
src/node_url.h Outdated
@@ -74,6 +74,16 @@ struct url_data {
std::string href;
};

namespace table_data {
extern const char* hex[256];
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
extern const char* hex[256];
extern const char* const hex[256];

(Aside -- using a single string of '%00\0%01\0%02\0...' and then accessing it as hex[index * 4] saves a bit more space and probably improves cache locality a bit.)

Copy link
Member Author

Choose a reason for hiding this comment

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

I've changed it to a single string. But to be honest, I think the readability isn't increased much.

image

Copy link
Member

Choose a reason for hiding this comment

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

Right – I didn’t say it would increase readability :)

@jasnell
Copy link
Member

jasnell commented Jun 11, 2021

As an alternative to a new *.cc file, these could be moved into a node_url-inl.h header

@XadillaX
Copy link
Member Author

As an alternative to a new *.cc file, these could be moved into a node_url-inl.h header

Because it's real data, not only declaration. If node_url-inl.h is mis-included by another file, the compilation will failed. So I think it's better to be stored in a *.cc file.

@RaisinTen
Copy link
Contributor

As an alternative to a new *.cc file, these could be moved into a node_url-inl.h header

Because it's real data, not only declaration. If node_url-inl.h is mis-included by another file, the compilation will failed. So I think it's better to be stored in a *.cc file.

Should we add a TODO comment to move the variables to node_url-inl.h when #38807 lands because in C++17 we can inline variables?

@addaleax
Copy link
Member

As an alternative to a new *.cc file, these could be moved into a node_url-inl.h header

Because it's real data, not only declaration. If node_url-inl.h is mis-included by another file, the compilation will failed. So I think it's better to be stored in a *.cc file.

Should we add a TODO comment to move the variables to node_url-inl.h when #38807 lands because in C++17 we can inline variables?

All we’d get out of that is probably just longer compilation times. @XadillaX is right, this is actual data that is accessed as data, so inlining has no benefit here and putting it in a .cc file is just the right thing to do, and I don’t understand why something else would be suggested here.

src/node_url.cc Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

XadillaX added a commit that referenced this pull request Jun 17, 2021
The purpose of separating is for readability and maintainability.

PR-URL: #38988
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
@XadillaX
Copy link
Member Author

Landed in 38a15d8

@XadillaX XadillaX closed this Jun 17, 2021
joyeecheung pushed a commit to joyeecheung/node that referenced this pull request Jun 17, 2021
The purpose of separating is for readability and maintainability.

PR-URL: nodejs#38988
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
danielleadams pushed a commit that referenced this pull request Jun 21, 2021
The purpose of separating is for readability and maintainability.

PR-URL: #38988
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
@danielleadams danielleadams mentioned this pull request Jun 21, 2021
richardlau pushed a commit that referenced this pull request Jul 19, 2021
The purpose of separating is for readability and maintainability.

PR-URL: #38988
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
richardlau pushed a commit that referenced this pull request Jul 20, 2021
The purpose of separating is for readability and maintainability.

PR-URL: #38988
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
@richardlau richardlau mentioned this pull request Jul 20, 2021
foxxyz pushed a commit to foxxyz/node that referenced this pull request Oct 18, 2021
The purpose of separating is for readability and maintainability.

PR-URL: nodejs#38988
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
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. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants