Skip to content

Conversation

XadillaX
Copy link
Contributor

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
Contributor 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
Contributor 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
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?

@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.

@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 <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
@XadillaX
Copy link
Contributor 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 <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
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 <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
@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 <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
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 <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
@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 <[email protected]>
Reviewed-By: Franziska Hinkelmann <[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. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants