src: fix and cleanup URL::SerializeURL #41759

addaleax · 2022-01-29T21:23:00Z

src: fix query/fragment serialization in URL::SerializeURL

These are presumably typos.

src: reserve string allocation space early in URL::SerializeURL

This can be useful for performance when doing many string
concatenations.

src: use const reference instead of pointer in URL::SerializeURL

Just some general cleanup to make things C++-y instead of C-y.

These are presumably typos.

This can be useful for performance when doing many string concatenations.

Just some general cleanup to make things C++-y instead of C-y.

nodejs-github-bot · 2022-01-29T21:27:21Z

CI: https://ci.nodejs.org/job/node-test-pull-request/42219/

src/node_url.cc

RaisinTen · 2022-01-30T13:25:26Z

cc @nodejs/url

src/node_url.cc

TimothyGu · 2022-01-30T22:51:57Z

src/node_url.cc

-  if (url->flags & URL_FLAGS_HAS_QUERY) {
-    output = "?" + url->query;
+  if (url.flags & URL_FLAGS_HAS_QUERY) {
+    output += "?" + url.query;


Typo indeed. Do we have tests for SerializeURL?

Not explicitly, I think. And I don't quite know where to put regression tests for this either.

Isn't this something that should be added to https://github.com/web-platform-tests/wpt/blob/master/url/resources/urltestdata.json?

src/node_url.cc

Co-authored-by: Timothy Gu <timothygu99@gmail.com>

nodejs-github-bot · 2022-01-31T14:43:16Z

CI: https://ci.nodejs.org/job/node-test-pull-request/42266/

RaisinTen

LGTM

nodejs-github-bot · 2022-02-11T11:40:14Z

Landed in ba5b5ac...33ffaa4

These are presumably typos. PR-URL: #41759 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>

This can be useful for performance when doing many string concatenations. PR-URL: #41759 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>

Just some general cleanup to make things C++-y instead of C-y. PR-URL: #41759 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>

These are presumably typos. PR-URL: nodejs#41759 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>

This can be useful for performance when doing many string concatenations. PR-URL: nodejs#41759 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>

Just some general cleanup to make things C++-y instead of C-y. PR-URL: nodejs#41759 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>