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: clean up WHATWG URL parser, round 2 #12251

Closed
wants to merge 4 commits into from

Conversation

@TimothyGu
Copy link
Member

commented Apr 6, 2017

This batch has some pretty heavy-duty changes, so reviewing commit-by-commit is probably preferred.

  • Reduce indentation

    For some reason the linter never caught it, but in all other source files namespaces do not affect indentation. Link to whitespace-free diff

  • Refactor node_url.h and node_url.cc

    A lot of inlined functions and lookup tables never needed to be in the header. Now that the header is going to be used more frequently in other areas of the codebase, it is best these things go into the actual implementation file.

  • Prefer templates over macros

    Many character checks were implemented as macros. Implement them as templatized inline functions for better type checking.

  • Do not export ARG_* flags in url binding

    They are not used by the JS layer.

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

src, url

src/node_url.cc Outdated
template <typename T, \
typename = typename std::enable_if<std::is_integral<T>::value && \
sizeof(T) >= n / 8, T>::type>

This comment has been minimized.

Copy link
@addaleax

addaleax Apr 9, 2017

Member

Tbh this does feel a bit like overkill ;) Plain template <typename T> static inlines should do the job, right?

This comment has been minimized.

Copy link
@addaleax

addaleax Apr 9, 2017

Member

Or, actually, as much as I love templates… maybe this is not the right time to use them? ;)

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Apr 15, 2017

Author Member

I'd still prefer to have some basic type-checking, but I agree the is_integral check is probably overkill. Opted for a static_assert on sizeof(T) instead.

src/node_url.cc Outdated
// Must return true if the character is to be percent-encoded
typedef bool (*must_escape_cb)(const unsigned char ch);
static inline bool BitAt(const uint8_t a[], const uint8_t i) {
return !!(a[i >> 3] & (1 << (i & 7)));

This comment has been minimized.

Copy link
@addaleax

addaleax Apr 9, 2017

Member

Fwiw you don’t need to do the !! if you’re returning a bool anyway

@joyeecheung joyeecheung added this to Spec Irrelavent / De-facto in WHATWG URL implementation Apr 11, 2017

@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 11, 2017

I have not yet had a chance to go into a full review but things look good so far.

@refack refack force-pushed the nodejs:master branch to fbe946b Apr 14, 2017

@addaleax

This comment has been minimized.

Copy link
Member

commented Apr 14, 2017

@TimothyGu This would need a rebase :)

@TimothyGu TimothyGu force-pushed the TimothyGu:url-cpp-cleanup branch 2 times, most recently Apr 15, 2017

@TimothyGu

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2017

@addaleax done 😺

TimothyGu added 2 commits Apr 6, 2017
src: do not export ARG_* flags in url binding
They are unused in the JS layer.

@TimothyGu TimothyGu force-pushed the TimothyGu:url-cpp-cleanup branch to e8f1dcc Apr 15, 2017

@TimothyGu

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2017

@watilde
Copy link
Member

left a comment

still LGTM :)

jasnell added a commit that referenced this pull request Apr 17, 2017
src: clean up WHATWG WG parser
* reduce indentation
* refactor URL inlined methods
* prefer templates over macros
* do not export ARG_* flags in url binding

PR-URL: #12251
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 17, 2017

Landed in ade80ee

@jasnell jasnell closed this Apr 17, 2017

@TimothyGu TimothyGu deleted the TimothyGu:url-cpp-cleanup branch Apr 17, 2017

@watilde watilde moved this from Spec Irrelavent / De-facto to Done in WHATWG URL implementation Apr 17, 2017

@TimothyGu TimothyGu referenced this pull request Apr 19, 2017
4 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
5 participants
You can’t perform that action at this time.