url: fix array overrun in node:url::SetArgs()

[anonrig]

I am following up on @tniessen's pull request and @bnoordhuis's comments. @TimothyGu, thanks for the mention & reminder

Reference: #46541 (comment)

[nodejs-github-bot]

Review requested:

• @nodejs/url

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[TimothyGu]

Small suggestion; otherwise lgtm.

[TimothyGu]

Ideally this parameter should be an const ada::url&. It doesn't make sense to pass an ada::result in when we always expect it to be filled with the "valid" value.

In fact the call sites should probably change from this:

  ada::result out = ada::parse(input.ToStringView());
  CHECK(out);
  out->set_protocol(…);

to

  ada::result out = ada::parse(input.ToStringView());
  CHECK(out);
  const ada::url& url = out.value();
  url.set_protocol(…);

[anonrig]

out.value() returns a copy. The current implementation looks unsafe, but CHECK(out) ensures that it is not. I tried to avoid returning a copy. @lemire wrote about this on Ada's discussion board: ada-url/ada#200

[TimothyGu]

I think you misunderstood ada-url/ada#200. out.value() (and equivalently *out) does not return a copy by itself. It returns a reference, so if you put it into a const ada::url& then no copies are made.

On the other hand, ada::url url = *out, which would make a copy. That's not what I'm suggesting here.

Alternatively, you can also do

  ada::result out = ada::parse(input.ToStringView());
  CHECK(out);
  ada::url url = std::move(*out);

which is written under the "performance tip". It would also avoid a copy.

[lemire]

It is likely that the current code is safe and efficient. I think we are discussing 'coding style' which is subjective.

I don't personally find the url->... annoying.

I am slightly triggered by the (*argv)[..] however. :-)

What about using a reference to an array instead?

Local<Value> (&argv)[10]

(The ampersand would be dropped from the calling site.)

[bnoordhuis]

It's a style guide thing: pointers for things that are mutated, const references otherwise.

With mutable references it's sometimes ambiguous to a reader if code operates on the original or on a copy. With pointers, no such ambiguity exists.

[RaisinTen]

The current fix LGTM but it feels like a C-style fix. Since this is a C++ file, we could also leverage the power of C++:

Since the problem described in #46541 (comment) is about array arguments decaying to pointers, why don't we use std::array here instead? The doc states:

Unlike a C-style array, it doesn't decay to T* automatically.

Doing that should also address https://github.com/nodejs/node/pull/47001/files#r1130218455:

I am slightly triggered by the (*argv)[..] however. :-)

[bnoordhuis]

@RaisinTen per convention, the std::array would have to be passed by pointer, so IMO it doesn't provide much benefit. More elaboration in #47001 (comment).

[joyeecheung]

Or you could just return a std::vector<Local<Value> and do Call(...., argv.size(), argv.data()) later ;) I doubt there is any meaningful performance difference between the two, and no one needs to actually count the number of arguments in the code that way, and you can get rid of the undef fillers in the caller.

[addaleax]

You could also just return a std::array? That would make intention clear better than an out parameters does, and would still avoid the dynamic memory allocation from std::vector (in fact, I'd expect it to compile to pretty much the same code as the current state of this PR).

[anonrig]

Since there isn't any request for changes on this pull request, I'll add a commit-queue label.

[nodejs-github-bot]

Landed in 72e971e

[danielleadams]

blocked by #46736