From b3cb8ce1113fd2c9b85f1051f53c17517a96a220 Mon Sep 17 00:00:00 2001 From: Myles Borins Date: Thu, 29 Mar 2018 16:17:27 -0400 Subject: [PATCH 01/13] 2018-03-29, Version 4.9.1 'Argon' (Maintenance) Notable changes: No additional commits. Due to incorrect staging of the upgrade to the GCC 4.9.X compiler, the latest releases for PPC little endian were built using GCC 4.9.X instead of GCC 4.8.X. This caused an ABI breakage on PPCLE based environments. This has been fixed in our infrastructure and we are doing this release to ensure that the hosted binaries are adhering to our platform support contract. PR-URL: https://github.com/nodejs/node/pull/19681 --- CHANGELOG.md | 3 ++- doc/changelogs/CHANGELOG_V4.md | 13 +++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e823878ec43..ca9c716a1b5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -109,7 +109,8 @@ release. 6.0.0
-4.9.0
+4.9.1
+4.9.0
4.8.7
4.8.6
4.8.5
diff --git a/doc/changelogs/CHANGELOG_V4.md b/doc/changelogs/CHANGELOG_V4.md index 7aaf7fdaadd..b006ad034d7 100644 --- a/doc/changelogs/CHANGELOG_V4.md +++ b/doc/changelogs/CHANGELOG_V4.md @@ -10,6 +10,7 @@ +4.9.1
4.9.0
4.8.7
4.8.6
@@ -71,6 +72,18 @@ [Node.js Long Term Support Plan](https://github.com/nodejs/LTS) and will be supported actively until April 2017 and maintained until April 2018. + +## 2018-03-29, Version 4.9.1 'Argon' (Maintenance), @MylesBorins + +### Notable Changes + +No additional commits. + +Due to incorrect staging of the upgrade to the GCC 4.9.X compiler, the latest releases for PPC little +endian were built using GCC 4.9.X instead of GCC 4.8.X. This caused an ABI breakage on PPCLE based +environments. This has been fixed in our infrastructure and we are doing this release to ensure that +the hosted binaries are adhering to our platform support contract. + ## 2018-03-28, Version 4.9.0 'Argon' (Maintenance), @MylesBorins From b5e9fcfa4e99bdb12f87dadcf773436f4cd0956b Mon Sep 17 00:00:00 2001 From: Myles Borins Date: Thu, 29 Mar 2018 16:11:10 -0400 Subject: [PATCH 02/13] 2018-03-29, Version 6.14.1 'Boron' (LTS) Notable changes: No additional commits. Due to incorrect staging of the upgrade to the GCC 4.9.X compiler, the latest releases for PPC little endian were built using GCC 4.9.X instead of GCC 4.8.X. This caused an ABI breakage on PPCLE based environments. This has been fixed in our infrastructure and we are doing this release to ensure that the hosted binaries are adhering to our platform support contract. PR-URL: https://github.com/nodejs/node/pull/19680 --- CHANGELOG.md | 3 ++- doc/changelogs/CHANGELOG_V6.md | 13 +++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ca9c716a1b5..d164a6240e2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -71,7 +71,8 @@ release. 8.0.0
-6.14.0
+6.14.1
+6.14.0
6.13.1
6.13.0
6.12.3
diff --git a/doc/changelogs/CHANGELOG_V6.md b/doc/changelogs/CHANGELOG_V6.md index 1c669800ddc..9dfd3da8378 100644 --- a/doc/changelogs/CHANGELOG_V6.md +++ b/doc/changelogs/CHANGELOG_V6.md @@ -10,6 +10,7 @@ +6.14.1
6.14.0
6.13.1
6.13.0
@@ -67,6 +68,18 @@ [Node.js Long Term Support Plan](https://github.com/nodejs/LTS) and will be supported actively until April 2018 and maintained until April 2019. + +## 2018-03-29, Version 6.14.1 'Boron' (LTS), @MylesBorins + +### Notable Changes + +No additional commits. + +Due to incorrect staging of the upgrade to the GCC 4.9.X compiler, the latest releases for PPC little +endian were built using GCC 4.9.X instead of GCC 4.8.X. This caused an ABI breakage on PPCLE based +environments. This has been fixed in our infrastructure and we are doing this release to ensure that +the hosted binaries are adhering to our platform support contract. + ## 2018-03-28, Version 6.14.0 'Boron' (LTS), @MylesBorins From ea4c69676af8000eb5bbd947fcee5b56fdfa4f9a Mon Sep 17 00:00:00 2001 From: Myles Borins Date: Thu, 29 Mar 2018 16:04:07 -0400 Subject: [PATCH 03/13] 2018-03-29, Version 8.11.1 'Carbon' (LTS) Notable changes: No additional commits. Due to incorrect staging of the upgrade to the GCC 4.9.X compiler, the latest releases for PPC little endian were built using GCC 4.9.X instead of GCC 4.8.X. This caused an ABI breakage on PPCLE based environments. This has been fixed in our infrastructure and we are doing this release to ensure that the hosted binaries are adhering to our platform support contract. Note that Node.js versions 10.X and later will be built with version 4.9.X or later of the GCC compiler, and it is possible that Node.js version 8.X may be built on the 4.9.X compiler at a later time as the stated minimum compiler requirement for Node.js version 8.X is 4.9.4. Refs: https://github.com/nodejs/node/blob/v8.x/BUILDING.md PR-URL: https://github.com/nodejs/node/pull/19679 --- CHANGELOG.md | 3 ++- doc/changelogs/CHANGELOG_V8.md | 19 +++++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d164a6240e2..43683115745 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -47,7 +47,8 @@ release. 9.0.0
-8.11.0
+8.11.1
+8.11.0
8.10.0
8.9.4
8.9.3
diff --git a/doc/changelogs/CHANGELOG_V8.md b/doc/changelogs/CHANGELOG_V8.md index 351c9190351..056ea93eef8 100644 --- a/doc/changelogs/CHANGELOG_V8.md +++ b/doc/changelogs/CHANGELOG_V8.md @@ -1,3 +1,4 @@ + # Node.js 8 ChangeLog @@ -10,6 +11,7 @@ +8.11.1
8.11.0
8.10.0
8.9.4
@@ -53,6 +55,23 @@ [Node.js Long Term Support Plan](https://github.com/nodejs/LTS) and will be supported actively until April 2019 and maintained until December 2019. + +## 2018-03-29, Version 8.11.1 'Carbon' (LTS), @MylesBorins + +### Notable Changes + +No additional commits. + +Due to incorrect staging of the upgrade to the GCC 4.9.X compiler, the latest releases for PPC little +endian were built using GCC 4.9.X instead of GCC 4.8.X. This caused an ABI breakage on PPCLE based +environments. This has been fixed in our infrastructure and we are doing this release to ensure that +the hosted binaries are adhering to our platform support contract. + +Note that Node.js versions 10.X and later will be built with version 4.9.X or later of the GCC compiler, +and it is possible that Node.js version 8.X may be built on the 4.9.X compiler at a later +time as the stated [minimum compiler requirement](https://github.com/nodejs/node/blob/v8.x/BUILDING.md) +for Node.js version 8.X is 4.9.4. + ## 2018-03-28, Version 8.11.0 'Carbon' (LTS), @MylesBorins From 1e0e988430383351a29c3f2e03c7e6497376123c Mon Sep 17 00:00:00 2001 From: Myles Borins Date: Thu, 29 Mar 2018 15:57:53 -0400 Subject: [PATCH 04/13] 2018-03-29, Version 9.10.1 (Current) Notable changes: No additional commits. Due to incorrect staging of the upgrade to the GCC 4.9.X compiler, the latest releases for PPC little endian were built using GCC 4.9.X instead of GCC 4.8.X. This caused an ABI breakage on PPCLE based environments. This has been fixed in our infrastructure and we are doing this release to ensure that the hosted binaries are adhering to our platform support contract. Note that Node.js versions 10.X and later will be built with version 4.9.X or later of the GCC compiler, and it is possible that Node.js version 9.X may be built on the 4.9.X compiler at a later time as the stated minimum compiler requirement for Node.js version 9.X is 4.9.4. Refs: https://github.com/nodejs/node/blob/v9.x/BUILDING.md PR-URL: https://github.com/nodejs/node/pull/19678 --- CHANGELOG.md | 3 ++- doc/changelogs/CHANGELOG_V9.md | 18 ++++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 43683115745..124ea510549 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,7 +31,8 @@ release. -9.10.0
+9.10.1
+9.10.0
9.9.0
9.8.0
9.7.1
diff --git a/doc/changelogs/CHANGELOG_V9.md b/doc/changelogs/CHANGELOG_V9.md index 47494d037c0..50cefbb97ba 100644 --- a/doc/changelogs/CHANGELOG_V9.md +++ b/doc/changelogs/CHANGELOG_V9.md @@ -9,6 +9,7 @@ +9.10.1
9.10.0
9.9.0
9.8.0
@@ -38,6 +39,23 @@ * [io.js](CHANGELOG_IOJS.md) * [Archive](CHANGELOG_ARCHIVE.md) + +## 2018-03-29, Version 9.10.1 (Current), @MylesBorins + +### Notable Changes + +No additional commits. + +Due to incorrect staging of the upgrade to the GCC 4.9.X compiler, the latest releases for PPC little +endian were built using GCC 4.9.X instead of GCC 4.8.X. This caused an ABI breakage on PPCLE based +environments. This has been fixed in our infrastructure and we are doing this release to ensure that +the hosted binaries are adhering to our platform support contract. + +Note that Node.js versions 10.X and later will be built with version 4.9.X or later of the GCC compiler, +and it is possible that Node.js version 9.X may be built on the 4.9.X compiler at a later +time as the stated [minimum compiler requirement](https://github.com/nodejs/node/blob/v8.x/BUILDING.md) +for Node.js version 9.X is 4.9.4. + ## 2018-03-28, Version 9.10.0 (Current), @MylesBorins prepared by @targos From 6c5144f4b1963ba2be7f9fde28d66aa3b9db729f Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Tue, 27 Mar 2018 21:34:43 -0700 Subject: [PATCH 05/13] doc: shorten character encoding introduction Keep the introduction for Buffers and character encodings short and to the point. The current introduction doesn't provide much in the way of useful additional information, but it is a bit confusing in its wording. ("such as" seems like it ought to refer to "encoded characters" but it actually refers to character encodings, which are not mentioned in the sentence. It may be arguable as to whether "hex-encoded" is in fact a character encoding, whether it should be stylized as "Hex-encoded" or not, and whether it should be spelled out as "Hexadecimal-encoded". None of that information is particularly useful to the end user at this point in the text. Omitting it simplifies and improves the documentation.) Additionally, the section is now wrapped to 80 characters. PR-URL: https://github.com/nodejs/node/pull/19648 Reviewed-By: James M Snell Reviewed-By: Gireesh Punathil Reviewed-By: Tiancheng "Timothy" Gu --- doc/api/buffer.md | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/doc/api/buffer.md b/doc/api/buffer.md index b551f72ad98..99c9c79d71d 100644 --- a/doc/api/buffer.md +++ b/doc/api/buffer.md @@ -149,10 +149,8 @@ changes: description: Removed the deprecated `raw` and `raws` encodings. --> -`Buffer` instances are commonly used to represent sequences of encoded characters -such as UTF-8, UCS2, Base64, or even Hex-encoded data. It is possible to -convert back and forth between `Buffer` instances and ordinary JavaScript strings -by using an explicit character encoding. +When string data is stored in or extracted out of a `Buffer` instance, a +character encoding may be specified. ```js const buf = Buffer.from('hello world', 'ascii'); @@ -161,6 +159,11 @@ console.log(buf.toString('hex')); // Prints: 68656c6c6f20776f726c64 console.log(buf.toString('base64')); // Prints: aGVsbG8gd29ybGQ= + +console.log(Buffer.from('fhqwhgads', 'ascii')); +// Prints: +console.log(Buffer.from('fhqwhgads', 'ucs2')); +// Prints: ``` The character encodings currently supported by Node.js include: From d37e59fa6aee7f5b38696726b0145741ef3eb95b Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Mon, 26 Mar 2018 13:35:08 +0100 Subject: [PATCH 06/13] stream: fix backpressure when multiple sync PR-URL: https://github.com/nodejs/node/pull/19613 Fixes: https://github.com/nodejs/node/issues/19601 Reviewed-By: James M Snell Reviewed-By: Trivikram Kamat Reviewed-By: Anna Henningsen --- lib/_stream_readable.js | 15 +++++++-- test/parallel/test-stream-backpressure.js | 39 +++++++++++++++++++++++ 2 files changed, 51 insertions(+), 3 deletions(-) create mode 100644 test/parallel/test-stream-backpressure.js diff --git a/lib/_stream_readable.js b/lib/_stream_readable.js index 9f79a07a6ff..7c6671fcd07 100644 --- a/lib/_stream_readable.js +++ b/lib/_stream_readable.js @@ -310,8 +310,7 @@ function chunkInvalid(state, chunk) { // 'readable' event will be triggered. function needMoreData(state) { return !state.ended && - (state.needReadable || - state.length < state.highWaterMark || + (state.length < state.highWaterMark || state.length === 0); } @@ -536,7 +535,17 @@ function emitReadable_(stream) { if (!state.destroyed && (state.length || state.ended)) { stream.emit('readable'); } - state.needReadable = !state.flowing && !state.ended; + + // The stream needs another readable event if + // 1. It is not flowing, as the flow mechanism will take + // care of it. + // 2. It is not ended. + // 3. It is below the highWaterMark, so we can schedule + // another readable later. + state.needReadable = + !state.flowing && + !state.ended && + state.length <= state.highWaterMark; flow(stream); } diff --git a/test/parallel/test-stream-backpressure.js b/test/parallel/test-stream-backpressure.js new file mode 100644 index 00000000000..03bcc233c87 --- /dev/null +++ b/test/parallel/test-stream-backpressure.js @@ -0,0 +1,39 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); +const stream = require('stream'); + +let pushes = 0; +const total = 65500 + 40 * 1024; +const rs = new stream.Readable({ + read: common.mustCall(function() { + if (pushes++ === 10) { + this.push(null); + return; + } + + const length = this._readableState.length; + + // We are at most doing two full runs of _reads + // before stopping, because Readable is greedy + // to keep its buffer full + assert(length <= total); + + this.push(Buffer.alloc(65500)); + for (let i = 0; i < 40; i++) { + this.push(Buffer.alloc(1024)); + } + + // We will be over highWaterMark at this point + // but a new call to _read is scheduled anyway. + }, 11) +}); + +const ws = stream.Writable({ + write: common.mustCall(function(data, enc, cb) { + setImmediate(cb); + }, 41 * 10) +}); + +rs.pipe(ws); From abc87862ff14c1571f008aa1a9cbf812bea9790c Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 16 Mar 2018 00:48:34 +0100 Subject: [PATCH 07/13] async_wrap: fix use-after-free for inspector session This fixes the following condition: $ python -u tools/run-valgrind.py ./node_g test/sequential/test-inspector-async-call-stack.js [...] ==10848== Invalid read of size 4 ==10848== at 0x12F509E: node::AsyncWrap::provider_type() const (async_wrap-inl.h:34) ==10848== by 0x12E7642: node::AsyncWrap::EmitTraceEventAfter() (async_wrap.cc:208) ==10848== by 0x12F301B: node::AsyncWrap::MakeCallback(v8::Local, int, v8::Local*) (async_wrap.cc:724) ==10848== by 0x14516C6: node::inspector::(anonymous namespace)::JSBindingsConnection::OnMessage(v8::Local) (inspector_js_api.cc:88) ==10848== by 0x14514F1: node::inspector::(anonymous namespace)::JSBindingsConnection::JSBindingsSessionDelegate::SendMessageToFrontend(v8_inspector::StringView const&) (inspector_js_api.cc:57) ==10848== by 0x14436AD: node::inspector::(anonymous namespace)::ChannelImpl::sendMessageToFrontend(v8_inspector::StringView const&) (inspector_agent.cc:232) ==10848== by 0x1443627: node::inspector::(anonymous namespace)::ChannelImpl::sendResponse(int, std::unique_ptr >) (inspector_agent.cc:221) ==10848== by 0x15C54EA: v8_inspector::V8InspectorSessionImpl::sendProtocolResponse(int, std::unique_ptr >) (v8-inspector-session-impl.cc:165) ==10848== by 0x14C1E81: v8_inspector::protocol::DispatcherBase::sendResponse(int, v8_inspector::protocol::DispatchResponse const&, std::unique_ptr >) (Protocol.cpp:660) ==10848== by 0x14C1F0A: v8_inspector::protocol::DispatcherBase::sendResponse(int, v8_inspector::protocol::DispatchResponse const&) (Protocol.cpp:665) ==10848== by 0x14E68E3: v8_inspector::protocol::Debugger::DispatcherImpl::setAsyncCallStackDepth(int, std::unique_ptr >, v8_inspector::protocol::ErrorSupport*) (Debugger.cpp:1353) ==10848== by 0x14E2D49: v8_inspector::protocol::Debugger::DispatcherImpl::dispatch(int, v8_inspector::String16 const&, std::unique_ptr >) (Debugger.cpp:920) ==10848== Address 0x64e6f88 is 24 bytes inside a block of size 80 free'd ==10848== at 0x4C3123B: operator delete(void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==10848== by 0x14534F8: node::inspector::(anonymous namespace)::JSBindingsConnection::~JSBindingsConnection() (inspector_js_api.cc:34) ==10848== by 0x145187E: node::inspector::(anonymous namespace)::JSBindingsConnection::Disconnect() (inspector_js_api.cc:111) ==10848== by 0x14518C9: node::inspector::(anonymous namespace)::JSBindingsConnection::Disconnect(v8::FunctionCallbackInfo const&) (inspector_js_api.cc:117) ==10848== by 0x166FF87: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo const&)) (api-arguments.cc:26) ==10848== by 0x172F829: v8::internal::MaybeHandle v8::internal::(anonymous namespace)::HandleApiCallHelper(v8::internal::Isolate*, v8::internal::Handle, v8::internal::Handle, v8::internal::Handle, v8::internal::Handle, v8::internal::BuiltinArguments) (builtins-api.cc:112) ==10848== by 0x172D85C: v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments, v8::internal::Isolate*) (builtins-api.cc:142) ==10848== by 0x172D5F6: v8::internal::Builtin_HandleApiCall(int, v8::internal::Object**, v8::internal::Isolate*) (builtins-api.cc:130) ==10848== by 0x7895E1842C3: ??? ==10848== by 0x7895E19B737: ??? ==10848== by 0x7895E19B737: ??? ==10848== by 0x7895E18F9C2: ??? ==10848== Block was alloc'd at ==10848== at 0x4C3017F: operator new(unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==10848== by 0x14517E8: node::inspector::(anonymous namespace)::JSBindingsConnection::New(v8::FunctionCallbackInfo const&) (inspector_js_api.cc:103) ==10848== by 0x166FF87: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo const&)) (api-arguments.cc:26) ==10848== by 0x172F113: v8::internal::MaybeHandle v8::internal::(anonymous namespace)::HandleApiCallHelper(v8::internal::Isolate*, v8::internal::Handle, v8::internal::Handle, v8::internal::Handle, v8::internal::Handle, v8::internal::BuiltinArguments) (builtins-api.cc:112) ==10848== by 0x172D748: v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments, v8::internal::Isolate*) (builtins-api.cc:138) ==10848== by 0x172D5F6: v8::internal::Builtin_HandleApiCall(int, v8::internal::Object**, v8::internal::Isolate*) (builtins-api.cc:130) ==10848== by 0x7895E1842C3: ??? ==10848== by 0x7895E1930DC: ??? ==10848== by 0x7895E293EAA: ??? ==10848== by 0x7895E19B737: ??? ==10848== by 0x7895E19B737: ??? ==10848== by 0x7895E19B737: ??? [...] PR-URL: https://github.com/nodejs/node/pull/19381 Reviewed-By: Eugene Ostroukhov Reviewed-By: Colin Ihrig Reviewed-By: James M Snell Reviewed-By: Tiancheng "Timothy" Gu --- src/async_wrap.cc | 13 ++++++++----- src/async_wrap.h | 2 +- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/async_wrap.cc b/src/async_wrap.cc index 90b532d73b8..de24239dd95 100644 --- a/src/async_wrap.cc +++ b/src/async_wrap.cc @@ -202,13 +202,13 @@ void AsyncWrap::EmitBefore(Environment* env, double async_id) { } -void AsyncWrap::EmitTraceEventAfter() { - switch (provider_type()) { +void AsyncWrap::EmitTraceEventAfter(ProviderType type, double async_id) { + switch (type) { #define V(PROVIDER) \ case PROVIDER_ ## PROVIDER: \ TRACE_EVENT_NESTABLE_ASYNC_END0( \ TRACING_CATEGORY_NODE1(async_hooks), \ - #PROVIDER "_CALLBACK", static_cast(get_async_id())); \ + #PROVIDER "_CALLBACK", static_cast(async_id)); \ break; NODE_ASYNC_PROVIDER_TYPES(V) #undef V @@ -314,7 +314,7 @@ static void PromiseHook(PromiseHookType type, Local promise, wrap->EmitTraceEventBefore(); AsyncWrap::EmitBefore(wrap->env(), wrap->get_async_id()); } else if (type == PromiseHookType::kAfter) { - wrap->EmitTraceEventAfter(); + wrap->EmitTraceEventAfter(wrap->provider_type(), wrap->get_async_id()); AsyncWrap::EmitAfter(wrap->env(), wrap->get_async_id()); if (env->execution_async_id() == wrap->get_async_id()) { // This condition might not be true if async_hooks was enabled during @@ -701,11 +701,14 @@ MaybeLocal AsyncWrap::MakeCallback(const Local cb, Local* argv) { EmitTraceEventBefore(); + ProviderType provider = provider_type(); async_context context { get_async_id(), get_trigger_async_id() }; MaybeLocal ret = InternalMakeCallback( env(), object(), cb, argc, argv, context); - EmitTraceEventAfter(); + // This is a static call with cached values because the `this` object may + // no longer be alive at this point. + EmitTraceEventAfter(provider, context.async_id); return ret; } diff --git a/src/async_wrap.h b/src/async_wrap.h index baaebb2a8b1..17017ab602c 100644 --- a/src/async_wrap.h +++ b/src/async_wrap.h @@ -142,7 +142,7 @@ class AsyncWrap : public BaseObject { static void EmitPromiseResolve(Environment* env, double async_id); void EmitTraceEventBefore(); - void EmitTraceEventAfter(); + static void EmitTraceEventAfter(ProviderType type, double async_id); void EmitTraceEventDestroy(); From 923fb5cc1861422291d135177770f94f473f4d6f Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 17 Mar 2018 17:52:57 +0100 Subject: [PATCH 08/13] net: track bytesWritten in C++ land Move tracking of `socket.bytesWritten` to C++ land. This makes it easier to provide this functionality for all `StreamBase` instances, and in particular should keep working when they have been 'consumed' in C++ in some way (e.g. for the network sockets that are underlying to TLS or HTTP2 streams). Also, this parallels `socket.bytesRead` a lot more now. PR-URL: https://github.com/nodejs/node/pull/19551 Reviewed-By: James M Snell --- lib/internal/net.js | 2 +- lib/net.js | 28 ++++++++++++++++++++-------- src/env.h | 1 + src/stream_base-inl.h | 28 +++++++++++++++++++++++++++- src/stream_base.cc | 1 + src/stream_base.h | 4 ++++ 6 files changed, 54 insertions(+), 10 deletions(-) diff --git a/lib/internal/net.js b/lib/internal/net.js index 9c2602b79e9..78e155e055a 100644 --- a/lib/internal/net.js +++ b/lib/internal/net.js @@ -32,7 +32,7 @@ function makeSyncWrite(fd) { if (enc !== 'buffer') chunk = Buffer.from(chunk, enc); - this._bytesDispatched += chunk.length; + this._handle.bytesWritten += chunk.length; const ctx = {}; writeBuffer(fd, chunk, 0, chunk.length, null, undefined, ctx); diff --git a/lib/net.js b/lib/net.js index aa5709981ca..5b460befa49 100644 --- a/lib/net.js +++ b/lib/net.js @@ -206,7 +206,6 @@ function normalizeArgs(args) { // called when creating new Socket, or when re-using a closed Socket function initSocketHandle(self) { self._undestroy(); - self._bytesDispatched = 0; self._sockname = null; // Handle creation may be deferred to bind() or connect() time. @@ -222,7 +221,8 @@ function initSocketHandle(self) { } -const BYTES_READ = Symbol('bytesRead'); +const kBytesRead = Symbol('kBytesRead'); +const kBytesWritten = Symbol('kBytesWritten'); function Socket(options) { @@ -278,6 +278,11 @@ function Socket(options) { this._writev = null; this._write = makeSyncWrite(fd); + // makeSyncWrite adjusts this value like the original handle would, so + // we need to let it do that by turning it into a writable, own property. + Object.defineProperty(this._handle, 'bytesWritten', { + value: 0, writable: true + }); } } else { // these will be set once there is a connection @@ -316,7 +321,8 @@ function Socket(options) { this._server = null; // Used after `.destroy()` - this[BYTES_READ] = 0; + this[kBytesRead] = 0; + this[kBytesWritten] = 0; } util.inherits(Socket, stream.Duplex); @@ -588,8 +594,9 @@ Socket.prototype._destroy = function(exception, cb) { if (this !== process.stderr) debug('close handle'); var isException = exception ? true : false; - // `bytesRead` should be accessible after `.destroy()` - this[BYTES_READ] = this._handle.bytesRead; + // `bytesRead` and `kBytesWritten` should be accessible after `.destroy()` + this[kBytesRead] = this._handle.bytesRead; + this[kBytesWritten] = this._handle.bytesWritten; this._handle.close(() => { debug('emit close'); @@ -689,7 +696,7 @@ function protoGetter(name, callback) { } protoGetter('bytesRead', function bytesRead() { - return this._handle ? this._handle.bytesRead : this[BYTES_READ]; + return this._handle ? this._handle.bytesRead : this[kBytesRead]; }); protoGetter('remoteAddress', function remoteAddress() { @@ -761,8 +768,6 @@ Socket.prototype._writeGeneric = function(writev, data, encoding, cb) { // Bail out if handle.write* returned an error if (ret) return ret; - this._bytesDispatched += req.bytes; - if (!req.async) { cb(); return; @@ -782,6 +787,13 @@ Socket.prototype._write = function(data, encoding, cb) { this._writeGeneric(false, data, encoding, cb); }; + +// Legacy alias. Having this is probably being overly cautious, but it doesn't +// really hurt anyone either. This can probably be removed safely if desired. +protoGetter('_bytesDispatched', function _bytesDispatched() { + return this._handle ? this._handle.bytesWritten : this[kBytesWritten]; +}); + protoGetter('bytesWritten', function bytesWritten() { var bytes = this._bytesDispatched; const state = this._writableState; diff --git a/src/env.h b/src/env.h index 19079aa5f07..75c530af454 100644 --- a/src/env.h +++ b/src/env.h @@ -117,6 +117,7 @@ struct PackageConfig { V(bytes_string, "bytes") \ V(bytes_parsed_string, "bytesParsed") \ V(bytes_read_string, "bytesRead") \ + V(bytes_written_string, "bytesWritten") \ V(cached_data_string, "cachedData") \ V(cached_data_produced_string, "cachedDataProduced") \ V(cached_data_rejected_string, "cachedDataRejected") \ diff --git a/src/stream_base-inl.h b/src/stream_base-inl.h index f4c228d7c59..35e49dfea2c 100644 --- a/src/stream_base-inl.h +++ b/src/stream_base-inl.h @@ -193,6 +193,10 @@ inline StreamWriteResult StreamBase::Write( v8::Local req_wrap_obj) { Environment* env = stream_env(); int err; + + for (size_t i = 0; i < count; ++i) + bytes_written_ += bufs[i].len; + if (send_handle == nullptr) { err = DoTryWrite(&bufs, &count); if (err != 0 || count == 0) { @@ -301,6 +305,12 @@ void StreamBase::AddMethods(Environment* env, env->as_external(), signature); + Local get_bytes_written_templ = + FunctionTemplate::New(env->isolate(), + GetBytesWritten, + env->as_external(), + signature); + t->PrototypeTemplate()->SetAccessorProperty(env->fd_string(), get_fd_templ, Local(), @@ -316,6 +326,11 @@ void StreamBase::AddMethods(Environment* env, Local(), attributes); + t->PrototypeTemplate()->SetAccessorProperty(env->bytes_written_string(), + get_bytes_written_templ, + Local(), + attributes); + env->SetProtoMethod(t, "readStart", JSMethod); env->SetProtoMethod(t, "readStop", JSMethod); if ((flags & kFlagNoShutdown) == 0) @@ -357,7 +372,6 @@ void StreamBase::GetFD(const FunctionCallbackInfo& args) { template void StreamBase::GetBytesRead(const FunctionCallbackInfo& args) { - // The handle instance hasn't been set. So no bytes could have been read. Base* handle; ASSIGN_OR_RETURN_UNWRAP(&handle, args.This(), @@ -368,6 +382,18 @@ void StreamBase::GetBytesRead(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(static_cast(wrap->bytes_read_)); } +template +void StreamBase::GetBytesWritten(const FunctionCallbackInfo& args) { + Base* handle; + ASSIGN_OR_RETURN_UNWRAP(&handle, + args.This(), + args.GetReturnValue().Set(0)); + + StreamBase* wrap = static_cast(handle); + // uint64_t -> double. 53bits is enough for all real cases. + args.GetReturnValue().Set(static_cast(wrap->bytes_written_)); +} + template void StreamBase::GetExternal(const FunctionCallbackInfo& args) { Base* handle; diff --git a/src/stream_base.cc b/src/stream_base.cc index 8838a1a6dfb..7b27a48c16f 100644 --- a/src/stream_base.cc +++ b/src/stream_base.cc @@ -243,6 +243,7 @@ int StreamBase::WriteString(const FunctionCallbackInfo& args) { uv_buf_t* bufs = &buf; size_t count = 1; err = DoTryWrite(&bufs, &count); + bytes_written_ += data_size; // Immediate failure or success if (err != 0 || count == 0) { diff --git a/src/stream_base.h b/src/stream_base.h index d5a759bd8de..4fe4a8c48c3 100644 --- a/src/stream_base.h +++ b/src/stream_base.h @@ -247,6 +247,7 @@ class StreamResource { StreamListener* listener_ = nullptr; uint64_t bytes_read_ = 0; + uint64_t bytes_written_ = 0; friend class StreamListener; }; @@ -324,6 +325,9 @@ class StreamBase : public StreamResource { template static void GetBytesRead(const v8::FunctionCallbackInfo& args); + template + static void GetBytesWritten(const v8::FunctionCallbackInfo& args); + template & args)> From 1dc8eb4bd34383830d48a704d79a2bc9ec55152f Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 24 Mar 2018 17:02:34 +0100 Subject: [PATCH 09/13] test: add regression test for large write Fixes: https://github.com/nodejs/node/issues/19562 PR-URL: https://github.com/nodejs/node/pull/19551 Reviewed-By: James M Snell --- test/parallel/test-net-bytes-written-large.js | 67 +++++++++++++++++++ 1 file changed, 67 insertions(+) create mode 100644 test/parallel/test-net-bytes-written-large.js diff --git a/test/parallel/test-net-bytes-written-large.js b/test/parallel/test-net-bytes-written-large.js new file mode 100644 index 00000000000..79a997ec5a3 --- /dev/null +++ b/test/parallel/test-net-bytes-written-large.js @@ -0,0 +1,67 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const net = require('net'); + +// Regression test for https://github.com/nodejs/node/issues/19562: +// Writing to a socket first tries to push through as much data as possible +// without blocking synchronously, and, if that is not enough, queues more +// data up for asynchronous writing. +// Check that `bytesWritten` accounts for both parts of a write. + +const N = 10000000; +{ + // Variant 1: Write a Buffer. + const server = net.createServer(common.mustCall((socket) => { + socket.end(Buffer.alloc(N), common.mustCall(() => { + assert.strictEqual(socket.bytesWritten, N); + })); + assert.strictEqual(socket.bytesWritten, N); + })).listen(0, common.mustCall(() => { + const client = net.connect(server.address().port); + client.resume(); + client.on('close', common.mustCall(() => { + assert.strictEqual(client.bytesRead, N); + server.close(); + })); + })); +} + +{ + // Variant 2: Write a string. + const server = net.createServer(common.mustCall((socket) => { + socket.end('a'.repeat(N), common.mustCall(() => { + assert.strictEqual(socket.bytesWritten, N); + })); + assert.strictEqual(socket.bytesWritten, N); + })).listen(0, common.mustCall(() => { + const client = net.connect(server.address().port); + client.resume(); + client.on('close', common.mustCall(() => { + assert.strictEqual(client.bytesRead, N); + server.close(); + })); + })); +} + +{ + // Variant 2: writev() with mixed data. + const server = net.createServer(common.mustCall((socket) => { + socket.cork(); + socket.write('a'.repeat(N)); + assert.strictEqual(socket.bytesWritten, N); + socket.write(Buffer.alloc(N)); + assert.strictEqual(socket.bytesWritten, 2 * N); + socket.end('', common.mustCall(() => { + assert.strictEqual(socket.bytesWritten, 2 * N); + })); + socket.uncork(); + })).listen(0, common.mustCall(() => { + const client = net.connect(server.address().port); + client.resume(); + client.on('close', common.mustCall(() => { + assert.strictEqual(client.bytesRead, 2 * N); + server.close(); + })); + })); +} From b7cfd278a53d2b7769340ed800142f6662aa48d2 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 24 Mar 2018 17:21:14 +0100 Subject: [PATCH 10/13] src: clean up `req.bytes` tracking Simply always tell the caller how many bytes were written, rather than letting them track it. In the case of writing a string, also keep track of the bytes written by the earlier `DoTryWrite()`. Refs: https://github.com/nodejs/node/issues/19562 PR-URL: https://github.com/nodejs/node/pull/19551 Reviewed-By: James M Snell --- src/stream_base-inl.h | 8 +++++--- src/stream_base.cc | 23 ++++++++++++----------- src/stream_base.h | 1 + 3 files changed, 18 insertions(+), 14 deletions(-) diff --git a/src/stream_base-inl.h b/src/stream_base-inl.h index 35e49dfea2c..392dc2c87c3 100644 --- a/src/stream_base-inl.h +++ b/src/stream_base-inl.h @@ -194,13 +194,15 @@ inline StreamWriteResult StreamBase::Write( Environment* env = stream_env(); int err; + size_t total_bytes = 0; for (size_t i = 0; i < count; ++i) - bytes_written_ += bufs[i].len; + total_bytes += bufs[i].len; + bytes_written_ += total_bytes; if (send_handle == nullptr) { err = DoTryWrite(&bufs, &count); if (err != 0 || count == 0) { - return StreamWriteResult { false, err, nullptr }; + return StreamWriteResult { false, err, nullptr, total_bytes }; } } @@ -230,7 +232,7 @@ inline StreamWriteResult StreamBase::Write( ClearError(); } - return StreamWriteResult { async, err, req_wrap }; + return StreamWriteResult { async, err, req_wrap, total_bytes }; } template diff --git a/src/stream_base.cc b/src/stream_base.cc index 7b27a48c16f..263943d2b03 100644 --- a/src/stream_base.cc +++ b/src/stream_base.cc @@ -60,12 +60,11 @@ int StreamBase::Shutdown(const FunctionCallbackInfo& args) { inline void SetWriteResultPropertiesOnWrapObject( Environment* env, Local req_wrap_obj, - const StreamWriteResult& res, - size_t bytes) { + const StreamWriteResult& res) { req_wrap_obj->Set( env->context(), env->bytes_string(), - Number::New(env->isolate(), bytes)).FromJust(); + Number::New(env->isolate(), res.bytes)).FromJust(); req_wrap_obj->Set( env->context(), env->async(), @@ -91,7 +90,6 @@ int StreamBase::Writev(const FunctionCallbackInfo& args) { MaybeStackBuffer bufs(count); size_t storage_size = 0; - uint32_t bytes = 0; size_t offset; if (!all_buffers) { @@ -123,7 +121,6 @@ int StreamBase::Writev(const FunctionCallbackInfo& args) { Local chunk = chunks->Get(i); bufs[i].base = Buffer::Data(chunk); bufs[i].len = Buffer::Length(chunk); - bytes += bufs[i].len; } } @@ -140,7 +137,6 @@ int StreamBase::Writev(const FunctionCallbackInfo& args) { if (Buffer::HasInstance(chunk)) { bufs[i].base = Buffer::Data(chunk); bufs[i].len = Buffer::Length(chunk); - bytes += bufs[i].len; continue; } @@ -160,12 +156,11 @@ int StreamBase::Writev(const FunctionCallbackInfo& args) { bufs[i].base = str_storage; bufs[i].len = str_size; offset += str_size; - bytes += str_size; } } StreamWriteResult res = Write(*bufs, count, nullptr, req_wrap_obj); - SetWriteResultPropertiesOnWrapObject(env, req_wrap_obj, res, bytes); + SetWriteResultPropertiesOnWrapObject(env, req_wrap_obj, res); if (res.wrap != nullptr && storage) { res.wrap->SetAllocatedStorage(storage.release(), storage_size); } @@ -193,7 +188,7 @@ int StreamBase::WriteBuffer(const FunctionCallbackInfo& args) { if (res.async) req_wrap_obj->Set(env->context(), env->buffer_string(), args[1]).FromJust(); - SetWriteResultPropertiesOnWrapObject(env, req_wrap_obj, res, buf.len); + SetWriteResultPropertiesOnWrapObject(env, req_wrap_obj, res); return res.err; } @@ -228,6 +223,7 @@ int StreamBase::WriteString(const FunctionCallbackInfo& args) { // Try writing immediately if write size isn't too big char stack_storage[16384]; // 16kb size_t data_size; + size_t synchronously_written = 0; uv_buf_t buf; bool try_write = storage_size <= sizeof(stack_storage) && @@ -243,7 +239,11 @@ int StreamBase::WriteString(const FunctionCallbackInfo& args) { uv_buf_t* bufs = &buf; size_t count = 1; err = DoTryWrite(&bufs, &count); - bytes_written_ += data_size; + // Keep track of the bytes written here, because we're taking a shortcut + // by using `DoTryWrite()` directly instead of using the utilities + // provided by `Write()`. + synchronously_written = count == 0 ? data_size : data_size - buf.len; + bytes_written_ += synchronously_written; // Immediate failure or success if (err != 0 || count == 0) { @@ -299,8 +299,9 @@ int StreamBase::WriteString(const FunctionCallbackInfo& args) { } StreamWriteResult res = Write(&buf, 1, send_handle, req_wrap_obj); + res.bytes += synchronously_written; - SetWriteResultPropertiesOnWrapObject(env, req_wrap_obj, res, data_size); + SetWriteResultPropertiesOnWrapObject(env, req_wrap_obj, res); if (res.wrap != nullptr) { res.wrap->SetAllocatedStorage(data.release(), data_size); } diff --git a/src/stream_base.h b/src/stream_base.h index 4fe4a8c48c3..dfce7df44a5 100644 --- a/src/stream_base.h +++ b/src/stream_base.h @@ -23,6 +23,7 @@ struct StreamWriteResult { bool async; int err; WriteWrap* wrap; + size_t bytes; }; From ae70e2bc34aca8b581f53eb49a27460ac1fc3f83 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 25 Mar 2018 15:02:23 +0200 Subject: [PATCH 11/13] src: general C++ cleanup in node_url.cc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Merge `domain` and `opaque` storage in URL parser: This just simplifies the code a bit, having multiple fields in an union with the same type is usually just overhead. - Add move variant of `URLHost::ToString()`: This helps avoid unnecessary string copy operations, especially since we control the lifetime of `URLHost` objects pretty well. - Use const refs in node_url.cc where appropriate - Remove or reduce overly generous `.reserve()` calls: These would otherwise keep a lot of unused memory lying around. - Return return values instead of unnecessary pointer arguments - Use more common/expressive variable names - Avoid macro use, reduce number of unnecessary JS strings: There’s no reason for `GET`, `GET_AND_SET` and `UTF8STRING` to be macros. Also, `GET` would previously create a JS string instance for each single call, even though the strings it was called with were compile-time constants. - Avoid unnecessary JS casts when the type of a value is known - Avoid (commonly unnecessary) copy for whitespace stripping PR-URL: https://github.com/nodejs/node/pull/19598 Reviewed-By: Tiancheng "Timothy" Gu Reviewed-By: James M Snell Reviewed-By: Daniel Bevenius --- src/env.h | 5 + src/node_url.cc | 338 +++++++++++++++++++++++++++--------------------- 2 files changed, 192 insertions(+), 151 deletions(-) diff --git a/src/env.h b/src/env.h index 75c530af454..a688b069242 100644 --- a/src/env.h +++ b/src/env.h @@ -165,11 +165,13 @@ struct PackageConfig { V(fingerprint_string, "fingerprint") \ V(fingerprint256_string, "fingerprint256") \ V(flags_string, "flags") \ + V(fragment_string, "fragment") \ V(get_data_clone_error_string, "_getDataCloneError") \ V(get_shared_array_buffer_id_string, "_getSharedArrayBufferId") \ V(gid_string, "gid") \ V(handle_string, "handle") \ V(homedir_string, "homedir") \ + V(host_string, "host") \ V(hostmaster_string, "hostmaster") \ V(ignore_string, "ignore") \ V(infoaccess_string, "infoAccess") \ @@ -226,6 +228,7 @@ struct PackageConfig { V(order_string, "order") \ V(owner_string, "owner") \ V(parse_error_string, "Parse Error") \ + V(password_string, "password") \ V(path_string, "path") \ V(pending_handle_string, "pendingHandle") \ V(pbkdf2_error_string, "PBKDF2 Error") \ @@ -239,6 +242,7 @@ struct PackageConfig { V(produce_cached_data_string, "produceCachedData") \ V(promise_string, "promise") \ V(pubkey_string, "pubkey") \ + V(query_string, "query") \ V(raw_string, "raw") \ V(read_host_object_string, "_readHostObject") \ V(readable_string, "readable") \ @@ -247,6 +251,7 @@ struct PackageConfig { V(rename_string, "rename") \ V(replacement_string, "replacement") \ V(retry_string, "retry") \ + V(scheme_string, "scheme") \ V(serial_string, "serial") \ V(scopeid_string, "scopeid") \ V(serial_number_string, "serialNumber") \ diff --git a/src/node_url.cc b/src/node_url.cc index 901fa0761a4..09199afb141 100644 --- a/src/node_url.cc +++ b/src/node_url.cc @@ -15,6 +15,7 @@ using v8::Context; using v8::Function; using v8::FunctionCallbackInfo; using v8::HandleScope; +using v8::Int32; using v8::Integer; using v8::Isolate; using v8::Local; @@ -26,23 +27,12 @@ using v8::TryCatch; using v8::Undefined; using v8::Value; -#define GET(env, obj, name) \ - obj->Get(env->context(), \ - OneByteString(env->isolate(), name)).ToLocalChecked() - -#define GET_AND_SET(env, obj, name, data, flag) \ - { \ - Local val = GET(env, obj, #name); \ - if (val->IsString()) { \ - Utf8Value value(env->isolate(), val.As()); \ - data->name = *value; \ - data->flags |= flag; \ - } \ - } - -#define UTF8STRING(isolate, str) \ - String::NewFromUtf8(isolate, str.c_str(), v8::NewStringType::kNormal) \ - .ToLocalChecked() +inline Local Utf8String(Isolate* isolate, const std::string& str) { + return String::NewFromUtf8(isolate, + str.data(), + v8::NewStringType::kNormal, + str.length()).ToLocalChecked(); +} namespace url { @@ -69,6 +59,8 @@ class URLHost { inline bool ParsingFailed() const { return type_ == HostType::H_FAILED; } std::string ToString() const; + // Like ToString(), but avoids a copy in exchange for invalidating `*this`. + std::string ToStringMove(); private: enum class HostType { @@ -80,10 +72,9 @@ class URLHost { }; union Value { - std::string domain; + std::string domain_or_opaque; uint32_t ipv4; uint16_t ipv6[8]; - std::string opaque; ~Value() {} Value() : ipv4(0) {} @@ -95,9 +86,12 @@ class URLHost { inline void Reset() { using string = std::string; switch (type_) { - case HostType::H_DOMAIN: value_.domain.~string(); break; - case HostType::H_OPAQUE: value_.opaque.~string(); break; - default: break; + case HostType::H_DOMAIN: + case HostType::H_OPAQUE: + value_.domain_or_opaque.~string(); + break; + default: + break; } type_ = HostType::H_FAILED; } @@ -113,13 +107,13 @@ class URLHost { inline void SetOpaque(std::string&& string) { Reset(); type_ = HostType::H_OPAQUE; - new(&value_.opaque) std::string(std::move(string)); + new(&value_.domain_or_opaque) std::string(std::move(string)); } inline void SetDomain(std::string&& string) { Reset(); type_ = HostType::H_DOMAIN; - new(&value_.domain) std::string(std::move(string)); + new(&value_.domain_or_opaque) std::string(std::move(string)); } }; @@ -136,7 +130,8 @@ URLHost::~URLHost() { XX(ARG_PORT) \ XX(ARG_PATH) \ XX(ARG_QUERY) \ - XX(ARG_FRAGMENT) + XX(ARG_FRAGMENT) \ + XX(ARG_COUNT) // This one has to be last. #define ERR_ARGS(XX) \ XX(ERR_ARG_FLAGS) \ @@ -665,7 +660,7 @@ inline std::string PercentDecode(const char* input, size_t len) { XX("ws:", 80) \ XX("wss:", 443) -inline bool IsSpecial(std::string scheme) { +inline bool IsSpecial(const std::string& scheme) { #define XX(name, _) if (scheme == name) return true; SPECIALS(XX); #undef XX @@ -684,7 +679,7 @@ inline bool StartsWithWindowsDriveLetter(const char* p, const char* end) { p[2] == '#'); } -inline int NormalizePort(std::string scheme, int p) { +inline int NormalizePort(const std::string& scheme, int p) { #define XX(name, port) if (scheme == name && p == port) return -1; SPECIALS(XX); #undef XX @@ -930,7 +925,7 @@ void URLHost::ParseIPv4Host(const char* input, size_t length, bool* is_ipv4) { void URLHost::ParseOpaqueHost(const char* input, size_t length) { CHECK_EQ(type_, HostType::H_FAILED); std::string output; - output.reserve(length * 3); + output.reserve(length); for (size_t i = 0; i < length; i++) { const char ch = input[i]; if (ch != '%' && IsForbiddenHostCodePoint(ch)) { @@ -1022,14 +1017,27 @@ inline T* FindLongestZeroSequence(T* values, size_t len) { return result; } +std::string URLHost::ToStringMove() { + std::string return_value; + switch (type_) { + case HostType::H_DOMAIN: + case HostType::H_OPAQUE: + return_value = std::move(value_.domain_or_opaque); + break; + default: + return_value = ToString(); + break; + } + Reset(); + return return_value; +} + std::string URLHost::ToString() const { std::string dest; switch (type_) { case HostType::H_DOMAIN: - return value_.domain; - break; case HostType::H_OPAQUE: - return value_.opaque; + return value_.domain_or_opaque; break; case HostType::H_IPV4: { dest.reserve(15); @@ -1089,103 +1097,125 @@ bool ParseHost(const std::string& input, host.ParseHost(input.c_str(), input.length(), is_special, unicode); if (host.ParsingFailed()) return false; - *output = host.ToString(); + *output = host.ToStringMove(); return true; } -inline void Copy(Environment* env, - Local ary, - std::vector* vec) { - const int32_t len = ary->Length(); +inline std::vector FromJSStringArray(Environment* env, + Local array) { + std::vector vec; + const int32_t len = array->Length(); if (len == 0) - return; // nothing to copy - vec->reserve(len); + return vec; // nothing to copy + vec.reserve(len); for (int32_t n = 0; n < len; n++) { - Local val = ary->Get(env->context(), n).ToLocalChecked(); + Local val = array->Get(env->context(), n).ToLocalChecked(); if (val->IsString()) { Utf8Value value(env->isolate(), val.As()); - vec->push_back(std::string(*value, value.length())); + vec.emplace_back(*value, value.length()); } } + return vec; } -inline Local Copy(Environment* env, - const std::vector& vec) { +inline Local ToJSStringArray(Environment* env, + const std::vector& vec) { Isolate* isolate = env->isolate(); - Local ary = Array::New(isolate, vec.size()); + Local array = Array::New(isolate, vec.size()); for (size_t n = 0; n < vec.size(); n++) - ary->Set(env->context(), n, UTF8STRING(isolate, vec[n])).FromJust(); - return ary; + array->Set(env->context(), n, Utf8String(isolate, vec[n])).FromJust(); + return array; } -inline void HarvestBase(Environment* env, - struct url_data* base, - Local base_obj) { +inline url_data HarvestBase(Environment* env, Local base_obj) { + url_data base; Local context = env->context(); - Local flags = GET(env, base_obj, "flags"); + Local flags = + base_obj->Get(env->context(), env->flags_string()).ToLocalChecked(); if (flags->IsInt32()) - base->flags = flags->Int32Value(context).FromJust(); - - Local scheme = GET(env, base_obj, "scheme"); - base->scheme = Utf8Value(env->isolate(), scheme).out(); - - GET_AND_SET(env, base_obj, username, base, URL_FLAGS_HAS_USERNAME); - GET_AND_SET(env, base_obj, password, base, URL_FLAGS_HAS_PASSWORD); - GET_AND_SET(env, base_obj, host, base, URL_FLAGS_HAS_HOST); - GET_AND_SET(env, base_obj, query, base, URL_FLAGS_HAS_QUERY); - GET_AND_SET(env, base_obj, fragment, base, URL_FLAGS_HAS_FRAGMENT); - Local port = GET(env, base_obj, "port"); + base.flags = flags->Int32Value(context).FromJust(); + + Local scheme = + base_obj->Get(env->context(), env->scheme_string()).ToLocalChecked(); + base.scheme = Utf8Value(env->isolate(), scheme).out(); + + auto GetStr = [&](std::string url_data::* member, + int flag, + Local name) { + Local value = base_obj->Get(env->context(), name).ToLocalChecked(); + if (value->IsString()) { + Utf8Value utf8value(env->isolate(), value.As()); + (base.*member).assign(*utf8value, utf8value.length()); + base.flags |= flag; + } + }; + GetStr(&url_data::username, URL_FLAGS_HAS_USERNAME, env->username_string()); + GetStr(&url_data::password, URL_FLAGS_HAS_PASSWORD, env->password_string()); + GetStr(&url_data::host, URL_FLAGS_HAS_HOST, env->host_string()); + GetStr(&url_data::query, URL_FLAGS_HAS_QUERY, env->query_string()); + GetStr(&url_data::fragment, URL_FLAGS_HAS_FRAGMENT, env->fragment_string()); + + Local port = + base_obj->Get(env->context(), env->port_string()).ToLocalChecked(); if (port->IsInt32()) - base->port = port->Int32Value(context).FromJust(); - Local path = GET(env, base_obj, "path"); + base.port = port.As()->Value(); + + Local + path = base_obj->Get(env->context(), env->path_string()).ToLocalChecked(); if (path->IsArray()) { - base->flags |= URL_FLAGS_HAS_PATH; - Copy(env, path.As(), &(base->path)); + base.flags |= URL_FLAGS_HAS_PATH; + base.path = FromJSStringArray(env, path.As()); } + return base; } -inline void HarvestContext(Environment* env, - struct url_data* context, - Local context_obj) { - Local flags = GET(env, context_obj, "flags"); +inline url_data HarvestContext(Environment* env, Local context_obj) { + url_data context; + Local flags = + context_obj->Get(env->context(), env->flags_string()).ToLocalChecked(); if (flags->IsInt32()) { - int32_t _flags = flags->Int32Value(env->context()).FromJust(); - if (_flags & URL_FLAGS_SPECIAL) - context->flags |= URL_FLAGS_SPECIAL; - if (_flags & URL_FLAGS_CANNOT_BE_BASE) - context->flags |= URL_FLAGS_CANNOT_BE_BASE; - if (_flags & URL_FLAGS_HAS_USERNAME) - context->flags |= URL_FLAGS_HAS_USERNAME; - if (_flags & URL_FLAGS_HAS_PASSWORD) - context->flags |= URL_FLAGS_HAS_PASSWORD; - if (_flags & URL_FLAGS_HAS_HOST) - context->flags |= URL_FLAGS_HAS_HOST; + static const int32_t copy_flags_mask = + URL_FLAGS_SPECIAL | + URL_FLAGS_CANNOT_BE_BASE | + URL_FLAGS_HAS_USERNAME | + URL_FLAGS_HAS_PASSWORD | + URL_FLAGS_HAS_HOST; + context.flags |= flags.As()->Value() & copy_flags_mask; } - Local scheme = GET(env, context_obj, "scheme"); + Local scheme = + context_obj->Get(env->context(), env->scheme_string()).ToLocalChecked(); if (scheme->IsString()) { Utf8Value value(env->isolate(), scheme); - context->scheme.assign(*value, value.length()); + context.scheme.assign(*value, value.length()); } - Local port = GET(env, context_obj, "port"); + Local port = + context_obj->Get(env->context(), env->port_string()).ToLocalChecked(); if (port->IsInt32()) - context->port = port->Int32Value(env->context()).FromJust(); - if (context->flags & URL_FLAGS_HAS_USERNAME) { - Local username = GET(env, context_obj, "username"); + context.port = port.As()->Value(); + if (context.flags & URL_FLAGS_HAS_USERNAME) { + Local username = + context_obj->Get(env->context(), + env->username_string()).ToLocalChecked(); CHECK(username->IsString()); Utf8Value value(env->isolate(), username); - context->username.assign(*value, value.length()); + context.username.assign(*value, value.length()); } - if (context->flags & URL_FLAGS_HAS_PASSWORD) { - Local password = GET(env, context_obj, "password"); + if (context.flags & URL_FLAGS_HAS_PASSWORD) { + Local password = + context_obj->Get(env->context(), + env->password_string()).ToLocalChecked(); CHECK(password->IsString()); Utf8Value value(env->isolate(), password); - context->password.assign(*value, value.length()); + context.password.assign(*value, value.length()); } - Local host = GET(env, context_obj, "host"); + Local host = + context_obj->Get(env->context(), + env->host_string()).ToLocalChecked(); if (host->IsString()) { Utf8Value value(env->isolate(), host); - context->host.assign(*value, value.length()); + context.host.assign(*value, value.length()); } + return context; } // Single dot segment can be ".", "%2e", or "%2E" @@ -1267,30 +1297,37 @@ void URL::Parse(const char* input, len = end - p; } + // The spec says we should strip out any ASCII tabs or newlines. + // In those cases, we create another std::string instance with the filtered + // contents, but in the general case we avoid the overhead. std::string whitespace_stripped; - whitespace_stripped.reserve(len); - for (const char* ptr = p; ptr < end; ptr++) + for (const char* ptr = p; ptr < end; ptr++) { if (!IsASCIITabOrNewline(*ptr)) - whitespace_stripped += *ptr; + continue; + // Hit tab or newline. Allocate storage, copy what we have until now, + // and then iterate and filter all similar characters out. + whitespace_stripped.reserve(len - 1); + whitespace_stripped.assign(p, ptr - p); + // 'ptr + 1' skips the current char, which we know to be tab or newline. + for (ptr = ptr + 1; ptr < end; ptr++) { + if (!IsASCIITabOrNewline(*ptr)) + whitespace_stripped += *ptr; + } - input = whitespace_stripped.c_str(); - len = whitespace_stripped.size(); - p = input; - end = input + len; + // Update variables like they should have looked like if the string + // had been stripped of whitespace to begin with. + input = whitespace_stripped.c_str(); + len = whitespace_stripped.size(); + p = input; + end = input + len; + break; + } - bool atflag = false; - bool sbflag = false; - bool uflag = false; + bool atflag = false; // Set when @ has been seen. + bool square_bracket_flag = false; // Set inside of [...] + bool password_token_seen_flag = false; // Set after a : after an username. std::string buffer; - url->scheme.reserve(len); - url->username.reserve(len); - url->password.reserve(len); - url->host.reserve(len); - url->path.reserve(len); - url->query.reserve(len); - url->fragment.reserve(len); - buffer.reserve(len); // Set the initial parse state. const bool has_state_override = state_override != kUnknownState; @@ -1347,7 +1384,7 @@ void URL::Parse(const char* input, // as it can be done before even entering C++ binding. } - url->scheme = buffer; + url->scheme = std::move(buffer); url->port = NormalizePort(url->scheme, url->port); if (new_is_special) { url->flags |= URL_FLAGS_SPECIAL; @@ -1373,7 +1410,7 @@ void URL::Parse(const char* input, } else { url->flags |= URL_FLAGS_CANNOT_BE_BASE; url->flags |= URL_FLAGS_HAS_PATH; - url->path.push_back(""); + url->path.emplace_back(""); state = kCannotBeBase; } } else if (!has_state_override) { @@ -1602,12 +1639,12 @@ void URL::Parse(const char* input, const char bch = buffer[n]; if (bch == ':') { url->flags |= URL_FLAGS_HAS_PASSWORD; - if (!uflag) { - uflag = true; + if (!password_token_seen_flag) { + password_token_seen_flag = true; continue; } } - if (uflag) { + if (password_token_seen_flag) { AppendOrEscape(&url->password, bch, USERINFO_ENCODE_SET); } else { AppendOrEscape(&url->username, bch, USERINFO_ENCODE_SET); @@ -1635,7 +1672,7 @@ void URL::Parse(const char* input, if (has_state_override && url->scheme == "file:") { state = kFileHost; continue; - } else if (ch == ':' && !sbflag) { + } else if (ch == ':' && !square_bracket_flag) { if (buffer.size() == 0) { url->flags |= URL_FLAGS_FAILED; return; @@ -1679,9 +1716,9 @@ void URL::Parse(const char* input, } } else { if (ch == '[') - sbflag = true; + square_bracket_flag = true; if (ch == ']') - sbflag = false; + square_bracket_flag = false; buffer += ch; } break; @@ -1888,12 +1925,12 @@ void URL::Parse(const char* input, ShortenUrlPath(url); if (ch != '/' && !special_back_slash) { url->flags |= URL_FLAGS_HAS_PATH; - url->path.push_back(""); + url->path.emplace_back(""); } } else if (IsSingleDotSegment(buffer) && ch != '/' && !special_back_slash) { url->flags |= URL_FLAGS_HAS_PATH; - url->path.push_back(""); + url->path.emplace_back(""); } else if (!IsSingleDotSegment(buffer)) { if (url->scheme == "file:" && url->path.empty() && @@ -1907,8 +1944,7 @@ void URL::Parse(const char* input, buffer[1] = ':'; } url->flags |= URL_FLAGS_HAS_PATH; - std::string segment(buffer.c_str(), buffer.size()); - url->path.push_back(segment); + url->path.emplace_back(std::move(buffer)); } buffer.clear(); if (url->scheme == "file:" && @@ -1947,7 +1983,7 @@ void URL::Parse(const char* input, case kQuery: if (ch == kEOL || (!has_state_override && ch == '#')) { url->flags |= URL_FLAGS_HAS_QUERY; - url->query = buffer; + url->query = std::move(buffer); buffer.clear(); if (ch == '#') state = kFragment; @@ -1959,7 +1995,7 @@ void URL::Parse(const char* input, switch (ch) { case kEOL: url->flags |= URL_FLAGS_HAS_FRAGMENT; - url->fragment = buffer; + url->fragment = std::move(buffer); break; case 0: break; @@ -1977,25 +2013,25 @@ void URL::Parse(const char* input, } // NOLINT(readability/fn_size) static inline void SetArgs(Environment* env, - Local argv[], - const struct url_data* url) { + Local argv[ARG_COUNT], + const struct url_data& url) { Isolate* isolate = env->isolate(); - argv[ARG_FLAGS] = Integer::NewFromUnsigned(isolate, url->flags); - argv[ARG_PROTOCOL] = OneByteString(isolate, url->scheme.c_str()); - if (url->flags & URL_FLAGS_HAS_USERNAME) - argv[ARG_USERNAME] = UTF8STRING(isolate, url->username); - if (url->flags & URL_FLAGS_HAS_PASSWORD) - argv[ARG_PASSWORD] = UTF8STRING(isolate, url->password); - if (url->flags & URL_FLAGS_HAS_HOST) - argv[ARG_HOST] = UTF8STRING(isolate, url->host); - if (url->flags & URL_FLAGS_HAS_QUERY) - argv[ARG_QUERY] = UTF8STRING(isolate, url->query); - if (url->flags & URL_FLAGS_HAS_FRAGMENT) - argv[ARG_FRAGMENT] = UTF8STRING(isolate, url->fragment); - if (url->port > -1) - argv[ARG_PORT] = Integer::New(isolate, url->port); - if (url->flags & URL_FLAGS_HAS_PATH) - argv[ARG_PATH] = Copy(env, url->path); + argv[ARG_FLAGS] = Integer::NewFromUnsigned(isolate, url.flags); + argv[ARG_PROTOCOL] = OneByteString(isolate, url.scheme.c_str()); + if (url.flags & URL_FLAGS_HAS_USERNAME) + argv[ARG_USERNAME] = Utf8String(isolate, url.username); + if (url.flags & URL_FLAGS_HAS_PASSWORD) + argv[ARG_PASSWORD] = Utf8String(isolate, url.password); + if (url.flags & URL_FLAGS_HAS_HOST) + argv[ARG_HOST] = Utf8String(isolate, url.host); + if (url.flags & URL_FLAGS_HAS_QUERY) + argv[ARG_QUERY] = Utf8String(isolate, url.query); + if (url.flags & URL_FLAGS_HAS_FRAGMENT) + argv[ARG_FRAGMENT] = Utf8String(isolate, url.fragment); + if (url.port > -1) + argv[ARG_PORT] = Integer::New(isolate, url.port); + if (url.flags & URL_FLAGS_HAS_PATH) + argv[ARG_PATH] = ToJSStringArray(env, url.path); } static void Parse(Environment* env, @@ -2015,12 +2051,12 @@ static void Parse(Environment* env, const bool has_context = context_obj->IsObject(); const bool has_base = base_obj->IsObject(); - struct url_data base; - struct url_data url; + url_data base; + url_data url; if (has_context) - HarvestContext(env, &url, context_obj.As()); + url = HarvestContext(env, context_obj.As()); if (has_base) - HarvestBase(env, &base, base_obj.As()); + base = HarvestBase(env, base_obj.As()); URL::Parse(input, len, state_override, &url, has_context, &base, has_base); if ((url.flags & URL_FLAGS_INVALID_PARSE_STATE) || @@ -2032,7 +2068,7 @@ static void Parse(Environment* env, const Local undef = Undefined(isolate); const Local null = Null(isolate); if (!(url.flags & URL_FLAGS_FAILED)) { - Local argv[9] = { + Local argv[] = { undef, undef, undef, @@ -2043,7 +2079,7 @@ static void Parse(Environment* env, null, // query defaults to null null, // fragment defaults to null }; - SetArgs(env, argv, &url); + SetArgs(env, argv, url); cb->Call(context, recv, arraysize(argv), argv).FromMaybe(Local()); } else if (error_cb->IsFunction()) { Local argv[2] = { undef, undef }; @@ -2152,7 +2188,7 @@ static void DomainToASCII(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(FIXED_ONE_BYTE_STRING(env->isolate(), "")); return; } - std::string out = host.ToString(); + std::string out = host.ToStringMove(); args.GetReturnValue().Set( String::NewFromUtf8(env->isolate(), out.c_str(), @@ -2172,7 +2208,7 @@ static void DomainToUnicode(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(FIXED_ONE_BYTE_STRING(env->isolate(), "")); return; } - std::string out = host.ToString(); + std::string out = host.ToStringMove(); args.GetReturnValue().Set( String::NewFromUtf8(env->isolate(), out.c_str(), @@ -2255,7 +2291,7 @@ const Local URL::ToObject(Environment* env) const { if (context_.flags & URL_FLAGS_FAILED) return Local(); - Local argv[9] = { + Local argv[] = { undef, undef, undef, @@ -2266,7 +2302,7 @@ const Local URL::ToObject(Environment* env) const { null, // query defaults to null null, // fragment defaults to null }; - SetArgs(env, argv, &context_); + SetArgs(env, argv, context_); MaybeLocal ret; { From b88477ef4dc82b2a77c90b5de65efab4cf507d3c Mon Sep 17 00:00:00 2001 From: Vse Mozhet Byt Date: Fri, 30 Mar 2018 13:38:45 +0300 Subject: [PATCH 12/13] tools: fix comment nits in tools/doc/*.js files * Unify first letters case. * Unify periods. * Delete excess spaces. * Add some blank lines as logical delimiters. * Remove obvious comments. * Combine short lines, rewrap lines more logically. * Fix typos. * "XXX" -> "TODO:", OSX -> macOS. PR-URL: https://github.com/nodejs/node/pull/19696 Reviewed-By: Ruben Bridgewater Reviewed-By: Shingo Inoue Reviewed-By: Richard Lau --- tools/doc/addon-verify.js | 4 +- tools/doc/common.js | 2 +- tools/doc/generate.js | 6 +-- tools/doc/html.js | 46 +++++++++++----------- tools/doc/json.js | 83 ++++++++++++++++++++------------------- tools/doc/type-parser.js | 4 +- 6 files changed, 73 insertions(+), 72 deletions(-) diff --git a/tools/doc/addon-verify.js b/tools/doc/addon-verify.js index 2e72abb77f9..9fcc71ed93f 100644 --- a/tools/doc/addon-verify.js +++ b/tools/doc/addon-verify.js @@ -47,7 +47,7 @@ function once(fn) { } function verifyFiles(files, blockName, onprogress, ondone) { - // must have a .cc and a .js to be a valid test + // Must have a .cc and a .js to be a valid test. if (!Object.keys(files).some((name) => /\.cc$/.test(name)) || !Object.keys(files).some((name) => /\.js$/.test(name))) { return; @@ -95,7 +95,7 @@ ${files[name].replace( }); fs.mkdir(dir, function() { - // Ignore errors + // Ignore errors. const done = once(ondone); var waiting = files.length; diff --git a/tools/doc/common.js b/tools/doc/common.js index 48ff3d1b828..553b52935fd 100644 --- a/tools/doc/common.js +++ b/tools/doc/common.js @@ -15,7 +15,7 @@ function extractAndParseYAML(text) { .replace(/^$/, ''); - // js-yaml.safeLoad() throws on error + // js-yaml.safeLoad() throws on error. const meta = yaml.safeLoad(text); if (meta.added) { diff --git a/tools/doc/generate.js b/tools/doc/generate.js index 1691db5ec00..0da9dba4e65 100644 --- a/tools/doc/generate.js +++ b/tools/doc/generate.js @@ -24,8 +24,8 @@ const processIncludes = require('./preprocess.js'); const fs = require('fs'); -// parse the args. -// Don't use nopt or whatever for this. It's simple enough. +// Parse the args. +// Don't use nopt or whatever for this. It's simple enough. const args = process.argv.slice(2); let format = 'json'; @@ -56,7 +56,7 @@ if (!filename) { fs.readFile(filename, 'utf8', (er, input) => { if (er) throw er; - // process the input for @include lines + // Process the input for @include lines. processIncludes(filename, input, next); }); diff --git a/tools/doc/html.js b/tools/doc/html.js index f2e7ed396b7..ff0230309ee 100644 --- a/tools/doc/html.js +++ b/tools/doc/html.js @@ -33,7 +33,7 @@ module.exports = toHTML; const STABILITY_TEXT_REG_EXP = /(.*:)\s*(\d)([\s\S]*)/; const DOC_CREATED_REG_EXP = //; -// customized heading without id attribute +// Customized heading without id attribute. const renderer = new marked.Renderer(); renderer.heading = function(text, level) { return `${text}\n`; @@ -42,7 +42,7 @@ marked.setOptions({ renderer: renderer }); -// TODO(chrisdickinson): never stop vomitting / fix this. +// TODO(chrisdickinson): never stop vomiting / fix this. const gtocPath = path.resolve(path.join( __dirname, '..', @@ -128,7 +128,7 @@ function render(opts, cb) { var { lexed, filename, template } = opts; const nodeVersion = opts.nodeVersion || process.version; - // get the section + // Get the section. const section = getSection(lexed); filename = path.basename(filename, '.md'); @@ -136,8 +136,8 @@ function render(opts, cb) { parseText(lexed); lexed = parseLists(lexed); - // generate the table of contents. - // this mutates the lexed contents in-place. + // Generate the table of contents. + // This mutates the lexed contents in-place. buildToc(lexed, filename, function(er, toc) { if (er) return cb(er); @@ -162,8 +162,8 @@ function render(opts, cb) { template = template.replace(/__ALTDOCS__/, altDocs(filename)); - // content has to be the last thing we do with - // the lexed tokens, because it's destructive. + // Content has to be the last thing we do with the lexed tokens, + // because it's destructive. const content = marked.parser(lexed); template = template.replace(/__CONTENT__/g, content); @@ -188,7 +188,7 @@ function analyticsScript(analytics) { `; } -// replace placeholders in text tokens +// Replace placeholders in text tokens. function replaceInText(text) { return linkJsTypeDocs(linkManPages(text)); } @@ -244,8 +244,8 @@ function altDocs(filename) { `; } -// handle general body-text replacements -// for example, link man page references to the actual page +// Handle general body-text replacements. +// For example, link man page references to the actual page. function parseText(lexed) { lexed.forEach(function(tok) { if (tok.type === 'table') { @@ -272,8 +272,8 @@ function parseText(lexed) { }); } -// just update the list item text in-place. -// lists that come right after a heading are what we're after. +// Just update the list item text in-place. +// Lists that come right after a heading are what we're after. function parseLists(input) { var state = null; const savedState = []; @@ -299,8 +299,8 @@ function parseLists(input) { const stabilityMatch = tok.text.match(STABILITY_TEXT_REG_EXP); const stability = Number(stabilityMatch[2]); const isStabilityIndex = - index - 2 === headingIndex || // general - index - 3 === headingIndex; // with api_metadata block + index - 2 === headingIndex || // General. + index - 3 === headingIndex; // With api_metadata block. if (heading && isStabilityIndex) { heading.stability = stability; @@ -408,17 +408,17 @@ function parseYAML(text) { return html.join('\n'); } -// Syscalls which appear in the docs, but which only exist in BSD / OSX +// Syscalls which appear in the docs, but which only exist in BSD / macOS. const BSD_ONLY_SYSCALLS = new Set(['lchmod']); -// Handle references to man pages, eg "open(2)" or "lchmod(2)" -// Returns modified text, with such refs replace with HTML links, for example -// 'open(2)' +// Handle references to man pages, eg "open(2)" or "lchmod(2)". +// Returns modified text, with such refs replaced with HTML links, for example +// 'open(2)'. function linkManPages(text) { return text.replace( /(^|\s)([a-z.]+)\((\d)([a-z]?)\)/gm, (match, beginning, name, number, optionalCharacter) => { - // name consists of lowercase letters, number is a single digit + // Name consists of lowercase letters, number is a single digit. const displayAs = `${name}(${number}${optionalCharacter})`; if (BSD_ONLY_SYSCALLS.has(name)) { return `${beginning}${text}\n`; @@ -68,20 +68,19 @@ function doJSON(input, filename, cb) { JSON.stringify(tok))); } - // Sometimes we have two headings with a single - // blob of description. Treat as a clone. + // Sometimes we have two headings with a single blob of description. + // Treat as a clone. if (current && state === 'AFTERHEADING' && depth === tok.depth) { var clone = current; current = newSection(tok); current.clone = clone; - // don't keep it around on the stack. + // Don't keep it around on the stack. stack.pop(); } else { - // if the level is greater than the current depth, - // then it's a child, so we should just leave the stack - // as it is. + // If the level is greater than the current depth, + // then it's a child, so we should just leave the stack as it is. // However, if it's a sibling or higher, then it implies // the closure of the other sections that came before. // root is always considered the level=0 section, @@ -99,19 +98,19 @@ function doJSON(input, filename, cb) { stack.push(current); state = 'AFTERHEADING'; return; - } // heading + } - // Immediately after a heading, we can expect the following + // Immediately after a heading, we can expect the following: // - // { type: 'blockquote_start' } + // { type: 'blockquote_start' }, // { type: 'paragraph', text: 'Stability: ...' }, - // { type: 'blockquote_end' } + // { type: 'blockquote_end' }, // - // a list: starting with list_start, ending with list_end, + // A list: starting with list_start, ending with list_end, // maybe containing other nested lists in each item. // - // If one of these isn't found, then anything that comes between - // here and the next heading should be parsed as the desc. + // If one of these isn't found, then anything that comes + // between here and the next heading should be parsed as the desc. var stability; if (state === 'AFTERHEADING') { if (type === 'blockquote_start') { @@ -171,7 +170,7 @@ function doJSON(input, filename, cb) { }); - // finish any sections left open + // Finish any sections left open. while (root !== (current = stack.pop())) { finishSection(current, stack[stack.length - 1]); } @@ -180,14 +179,15 @@ function doJSON(input, filename, cb) { } -// go from something like this: +// Go from something like this: +// // [ { type: 'list_item_start' }, // { type: 'text', // text: '`settings` Object, Optional' }, // { type: 'list_start', ordered: false }, // { type: 'list_item_start' }, // { type: 'text', -// text: 'exec: String, file path to worker file. Default: `__filename`' }, +// text: 'exec: String, file path to worker file. Default: `__filename`' }, // { type: 'list_item_end' }, // { type: 'list_item_start' }, // { type: 'text', @@ -204,7 +204,9 @@ function doJSON(input, filename, cb) { // { type: 'list_end' }, // { type: 'list_item_end' }, // { type: 'list_end' } ] +// // to something like: +// // [ { name: 'settings', // type: 'object', // optional: true, @@ -228,7 +230,7 @@ function processList(section) { var current; const stack = []; - // for now, *just* build the heirarchical list + // For now, *just* build the hierarchical list. list.forEach(function(tok) { const type = tok.type; if (type === 'space') return; @@ -260,23 +262,23 @@ function processList(section) { } }); - // shove the name in there for properties, since they are always - // just going to be the value etc. + // Shove the name in there for properties, + // since they are always just going to be the value etc. if (section.type === 'property' && values[0]) { values[0].textRaw = `\`${section.name}\` ${values[0].textRaw}`; } - // now pull the actual values out of the text bits. + // Now pull the actual values out of the text bits. values.forEach(parseListItem); // Now figure out what this list actually means. - // depending on the section type, the list could be different things. + // Depending on the section type, the list could be different things. switch (section.type) { case 'ctor': case 'classMethod': case 'method': - // each item is an argument, unless the name is 'return', + // Each item is an argument, unless the name is 'return', // in which case it's the return value. section.signatures = section.signatures || []; var sig = {}; @@ -292,8 +294,8 @@ function processList(section) { break; case 'property': - // there should be only one item, which is the value. - // copy the data up to the section. + // There should be only one item, which is the value. + // Copy the data up to the section. var value = values[0] || {}; delete value.name; section.typeof = value.type || section.typeof; @@ -304,7 +306,7 @@ function processList(section) { break; case 'event': - // event: each item is an argument. + // Event: each item is an argument. section.params = values; break; @@ -336,7 +338,7 @@ function parseSignature(text, sig) { var optional = false; var def; - // for grouped optional params such as someMethod(a[, b[, c]]) + // For grouped optional params such as someMethod(a[, b[, c]]). var pos; for (pos = 0; pos < p.length; pos++) { if (optionalCharDict[p[pos]] === undefined) { break; } @@ -358,7 +360,7 @@ function parseSignature(text, sig) { if (!param) { param = sig.params[i] = { name: p }; } - // at this point, the name should match. + // At this point, the name should match. if (p !== param.name) { console.error('Warning: invalid param "%s"', p); console.error(` > ${JSON.stringify(param)}`); @@ -374,8 +376,8 @@ function parseListItem(item) { if (item.options) item.options.forEach(parseListItem); if (!item.textRaw) return; - // the goal here is to find the name, type, default, and optional. - // anything left over is 'desc' + // The goal here is to find the name, type, default, and optional. + // Anything left over is 'desc'. var text = item.textRaw.trim(); // text = text.replace(/^(Argument|Param)s?\s*:?\s*/i, ''); @@ -449,9 +451,8 @@ function finishSection(section, parent) { if (!section.list) section.list = []; processList(section); - // classes sometimes have various 'ctor' children - // which are actually just descriptions of a constructor - // class signature. + // Classes sometimes have various 'ctor' children + // which are actually just descriptions of a constructor class signature. // Merge them into the parent. if (section.type === 'class' && section.ctors) { section.signatures = section.signatures || []; @@ -466,8 +467,8 @@ function finishSection(section, parent) { delete section.ctors; } - // properties are a bit special. - // their "type" is the type of object, not "property" + // Properties are a bit special. + // Their "type" is the type of object, not "property". if (section.properties) { section.properties.forEach(function(p) { if (p.typeof) p.type = p.typeof; @@ -476,7 +477,7 @@ function finishSection(section, parent) { }); } - // handle clones + // Handle clones. if (section.clone) { var clone = section.clone; delete section.clone; @@ -494,7 +495,7 @@ function finishSection(section, parent) { plur = `${section.type}s`; } - // if the parent's type is 'misc', then it's just a random + // If the parent's type is 'misc', then it's just a random // collection of stuff, like the "globals" section. // Make the children top-level items. if (section.type === 'misc') { @@ -554,7 +555,7 @@ function deepCopy_(src) { } -// these parse out the contents of an H# tag +// These parse out the contents of an H# tag. const eventExpr = /^Event(?::|\s)+['"]?([^"']+).*$/i; const classExpr = /^Class:\s*([^ ]+).*$/i; const propExpr = /^[^.]+\.([^ .()]+)\s*$/; @@ -566,7 +567,7 @@ var paramExpr = /\((.*)\);?$/; function newSection(tok) { const section = {}; - // infer the type from the text. + // Infer the type from the text. const text = section.textRaw = tok.text; if (text.match(eventExpr)) { section.type = 'event'; diff --git a/tools/doc/type-parser.js b/tools/doc/type-parser.js index dbf1538c10f..1cbd1a5552b 100644 --- a/tools/doc/type-parser.js +++ b/tools/doc/type-parser.js @@ -5,7 +5,7 @@ const jsDocPrefix = 'https://developer.mozilla.org/en-US/docs/Web/JavaScript/'; const jsPrimitiveUrl = `${jsDocPrefix}Data_structures`; const jsPrimitives = { 'boolean': 'Boolean', - 'integer': 'Number', // not a primitive, used for clarification + 'integer': 'Number', // Not a primitive, used for clarification. 'null': 'Null', 'number': 'Number', 'string': 'String', @@ -110,7 +110,7 @@ function toLink(typeInput) { let typeUrl = null; // To support type[], type[][] etc., we store the full string - // and use the bracket-less version to lookup the type URL + // and use the bracket-less version to lookup the type URL. const typeTextFull = typeText; typeText = typeText.replace(arrayPart, ''); From 83d44bee0134e3b8b6b98bf89e1f02981d72adee Mon Sep 17 00:00:00 2001 From: Vse Mozhet Byt Date: Fri, 30 Mar 2018 12:28:34 +0300 Subject: [PATCH 13/13] tools: dry utility function in tools/doc/json.js MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Also, move a declaration of unrelated variable closer to its only context. PR-URL: https://github.com/nodejs/node/pull/19692 Reviewed-By: Ruben Bridgewater Reviewed-By: Tobias Nießen --- tools/doc/json.js | 55 ++++++++++++++++------------------------------- 1 file changed, 19 insertions(+), 36 deletions(-) diff --git a/tools/doc/json.js b/tools/doc/json.js index 6eaee8c7fc1..87122cea70f 100644 --- a/tools/doc/json.js +++ b/tools/doc/json.js @@ -323,6 +323,8 @@ function processList(section) { delete section.list; } +const paramExpr = /\((.*)\);?$/; + // textRaw = "someobject.someMethod(a[, b=100][, c])" function parseSignature(text, sig) { var params = text.match(paramExpr); @@ -556,42 +558,23 @@ function deepCopy_(src) { // These parse out the contents of an H# tag. -const eventExpr = /^Event(?::|\s)+['"]?([^"']+).*$/i; -const classExpr = /^Class:\s*([^ ]+).*$/i; -const propExpr = /^[^.]+\.([^ .()]+)\s*$/; -const braceExpr = /^[^.[]+(\[[^\]]+\])\s*$/; -const classMethExpr = /^class\s*method\s*:?[^.]+\.([^ .()]+)\([^)]*\)\s*$/i; -const methExpr = /^(?:[^.]+\.)?([^ .()]+)\([^)]*\)\s*$/; -const newExpr = /^new ([A-Z][a-zA-Z]+)\([^)]*\)\s*$/; -var paramExpr = /\((.*)\);?$/; - -function newSection(tok) { - const section = {}; +const headingExpressions = [ + { type: 'event', re: /^Event(?::|\s)+['"]?([^"']+).*$/i }, + { type: 'class', re: /^Class:\s*([^ ]+).*$/i }, + { type: 'property', re: /^[^.[]+(\[[^\]]+\])\s*$/ }, + { type: 'property', re: /^[^.]+\.([^ .()]+)\s*$/ }, + { type: 'classMethod', re: /^class\s*method\s*:?[^.]+\.([^ .()]+)\([^)]*\)\s*$/i }, + { type: 'method', re: /^(?:[^.]+\.)?([^ .()]+)\([^)]*\)\s*$/ }, + { type: 'ctor', re: /^new ([A-Z][a-zA-Z]+)\([^)]*\)\s*$/ }, +]; + +function newSection({ text }) { // Infer the type from the text. - const text = section.textRaw = tok.text; - if (text.match(eventExpr)) { - section.type = 'event'; - section.name = text.replace(eventExpr, '$1'); - } else if (text.match(classExpr)) { - section.type = 'class'; - section.name = text.replace(classExpr, '$1'); - } else if (text.match(braceExpr)) { - section.type = 'property'; - section.name = text.replace(braceExpr, '$1'); - } else if (text.match(propExpr)) { - section.type = 'property'; - section.name = text.replace(propExpr, '$1'); - } else if (text.match(classMethExpr)) { - section.type = 'classMethod'; - section.name = text.replace(classMethExpr, '$1'); - } else if (text.match(methExpr)) { - section.type = 'method'; - section.name = text.replace(methExpr, '$1'); - } else if (text.match(newExpr)) { - section.type = 'ctor'; - section.name = text.replace(newExpr, '$1'); - } else { - section.name = text; + for (const { type, re } of headingExpressions) { + const [, name] = text.match(re) || []; + if (name) { + return { textRaw: text, type, name }; + } } - return section; + return { textRaw: text, name: text }; }