url: build warning fixes

[brodycj]

Checklist

• make -j8 test (UNIX [macOS]; Linux i386 & amd64) and vcbuild test nosign (.\vcbuild.bat nosign then .\vcbuild.bat test nosign nobuild as Administrator on Windows) PASS OK
• commit message follows commit guidelines

Additional item

• I hereby certify the statements in Developer's Certificate of Origin 1.1

Affected core subsystem(s)

url

Description of change

• mark ignored return value in node::url::Parse(...) (originally in Fix unused result warning in node_url.cc (nit) #9757)
• use URL_FAILED instead of FAILED macro to resolve macro redefinition warning on Windows

[brodycj]

NOTE: This was originally a part of PR #10139.

[sam-github]

How about just an #undef FAILED? It would decrease churn in the code below.

[brodycj]

In src/node_url.cc: > @@ -62,7 +62,7 @@ using v8::Value; url.flags |= URL_FLAGS_TERMINATED; \

goto done; \ } -#define FAILED() \ +#define URL_FAILED() \

How about just an #undef FAILED? It would decrease churn in the code

below. Thanks @addaleax and @sam-github for the feedback! +1 for touching less code. In general: I don't really like undefining and redefining macros since the meaning changes depending on what part of the code it is in. I think this is an ugly solution for an ugly problem. -1 I would personally favor reducing the ugliness here by using something like URL_FAILED. I would like to leave this open for any other comments and would like to follow a majority opinion from Node.js owners here. Hoping to avoid excess bikeshedding.

[sam-github]

OK, I'm fine with the macro rename. LGTM

[mhdawson]

LGTM

[brodycj]

Thanks guys!

[brodycj]

I realized the following change in #10145 should have been part of this PR to resolve a conversion warning on the Windows build:

diff --git a/src/node_url.cc b/src/node_url.cc
index 7502461..b048449 100644
--- a/src/node_url.cc
+++ b/src/node_url.cc
@@ -341,7 +341,8 @@ namespace url {
       val = numbers[parts - 1];
       for (int n = 0; n < parts - 1; n++) {
         double b = 3-n;
-        val += numbers[n] * pow(256, b);
+        // TBD POSSIBLE DATA LOSS:
+        val += static_cast<uint32_t>(numbers[n] * pow(256, b));
       }
     }
 

I would be happy to take one of the following actions:

1. combine with 5906faa ("url: mark ignored return value in node::url::Parse(...)") and rebase along with 9695066 (fix src/node_url.cc to resolve the macro redefinition warning on Windows)
2. add a new commit with this fix to this PR without rebasing
3. open a new PR to resolve the conversion warning on Windows

@sam-github @addaleax @mhdawson please let me know which action I should take. Thanks!

[addaleax]

I don’t think there’s any semantic relation to any of the commits in this PR, so feel free to open a new one

[jasnell]

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

[jasnell]

Landed in 595b22a

[MylesBorins]

This does not land cleanly in LTS. Added dont-land label. Please feel free to manually backport