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

dgram: use strict equality comparison #2702

Closed
wants to merge 28 commits into from
Closed

dgram: use strict equality comparison #2702

wants to merge 28 commits into from

Conversation

JungMinu
Copy link
Member

@JungMinu JungMinu commented Sep 5, 2015

There is no type-conversion to be done because both parameters are already the same type.
Therefore, the === operator should be used for better performance.

skomski and others added 28 commits September 2, 2015 18:39
v8 will silently return an empty handle
which doesn't delete our data if string length is
above String::kMaxLength

Fixes: #1374
PR-URL: #2402
Reviewed-By: trevnorris - Trevor Norris <trev.norris@gmail.com>
Reviewed-By: indutny - Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
Add an optional callback parameter to `ChildProcess.prototype.send()`
that is invoked when the message has been sent.

Juggle the control channel's reference count so that in-flight messages
keep the event loop (and therefore the process) alive until they have
been sent.

`ChildProcess.prototype.send()` and `process.send()` used to operate
synchronously but became asynchronous in commit libuv/libuv@393c1c5
("unix: set non-block mode in uv_{pipe,tcp,udp}_open"), which landed
in io.js in commit 07bd05b ("deps: update libuv to 1.2.1").

Fixes: #760
PR-URL: #2620
Reviewed-By: trevnorris - Trevor Norris <trev.norris@gmail.com>
Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>
This test has failed recently during a PR test in Jenkins, for
reasons seemingly not related to the change in the PR.

PR-URL: #2648
Reviewed-By: evanlucas - Evan Lucas <evanlucas@me.com>
Fixes: #1972
PR-URL: #2322
Reviewed-By: orangemocha - Alexis Campailla <orangemocha@nodejs.org>
Reviewed-By: thefourtheye - Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Fixes: #2113
Ref: 17a379e
PR-URL: #2605
Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>
Reviewed-By: cjihrig - Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: mscdex - Brian White <mscdex@mscdex.net>
This retains the key elements of test-child-process-fork-getconnections
(forks a child process, sends a bunch of sockets, uses getConnections()
to enumerate them) but contains some code to work around an apparent
intermittent bug that occurs on OS X where a socket seems to close
itself unexpectedly.

#2610 was opened for the bug that
was causing the problem in the first place.

PR-URL: #2609
Fixes: #1100
Reviewed-By: jbergstroem - Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Brendan Ashworth <brendan.ashworth@me.com>
Port of joyent/node commits:

 * nodejs/node-v0.x-archive@e17c5a7
 * nodejs/node-v0.x-archive@70dafa7

Pull over test-child-process-spawn-typeerror.js from v0.12, replacing
the existing test in master. The new test includes a broader set of
tests on the various arg choices and throws.

Reviewed-By: trevnorris - Trevor Norris <trevnorris@nodejs.org>
Reviewed-By: cjihrig - Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: thefourtheye - Sakthipriyan Vairamani
PR-URL: #2667
Fixes: #2515
PR-URL: #2376
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Now parts of our public and public-ish APIs fall back to old-style
listenerCount() if the emitter does not have a listenerCount function.

Fixes: #2655
Refs: 8f58fb9

PR-URL: #2661
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Upgrade to the latest branch-head for V8 4.5. For the full commit log see
https://github.com/v8/v8-git-mirror/commits/4.5.103.24

PR-URL: #2509
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Apply the src/node_contextify.cc and lib/module.js fixups from @bnoordhuis
41e63fb

PR-URL: #2509
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
The `context_` is not initialized until the `CreateV8Context` will
return. Make sure that it will be empty (by moving away initialization
from constructor) at start, and ignore getter callbacks until it will
have some value.

PR-URL: #2091
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
The list of Array properties needed to be updated to match the new ones added
in V8 4.5.

PR-URL: #2509
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Turn on V8 API deprecation warnings.  Fix up the no-arg Isolate::New()
calls in src/node.cc and src/debug-agent.cc.

PR-URL: #2091
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
v8::Handle is deprecated: https://codereview.chromium.org/1224623004

PR-URL: #2202
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
The flag is no longer supported by V8 4.5, and the original issue [1] on ARMv6
no longer manifests with (at least) 4.5.103.20.

[1] See https://code.google.com/p/v8/issues/detail?id=4338

PR-URL: #2509
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Ref: #1376
Ref: #1398
Issue fixed in V8: https://chromium.googlesource.com/v8/v8/+/81703350bbb9923d211fe5b79e90bd458b0916e2
V8-Bug: https://code.google.com/p/v8/issues/detail?id=4019

PR-URL: #2592
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Pick up v8/v8@f9a0a16
Commit log at https://chromium.googlesource.com/v8/v8.git/+log/branch-heads/4.5

PR-URL: #2632
Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: rvagg - Rod Vagg <rod@vagg.org>
PR-URL: #2685
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Fixes a regression that appeared with the new Buffer implementation in v3.
Without this change, calling the SlowBuffer constructor with something else
than a number would abort on the C++ side. This makes sure that the length
argument is coerced to number or is 0.

Fixes: #2634
PR-URL: #2635
Reviewed-By: trevnorris - Trevor Norris <trev.norris@gmail.com>
Reviewed-By: cjihrig - Colin Ihrig <cjihrig@gmail.com>
Recursive file watching is supported by libuv since 1.7.0. Refer
https://github.com/nodejs/node/blob/master/deps/uv/ChangeLog#L126. This
patch notes that in the docs and enables testing this feature. It also
adds proper TAP plugin parsable message for other platforms.

PR-URL: #2649
Fixes: #375
Reviewed-By: rvagg - Rod Vagg <rod@vagg.org>
Reviewed-By: silverwind - Roman Reiss <me@silverwind.io>
Long exception lines resulted in a stack buffer overflow or assertion
because it was assumed snprintf not counts discarded chars
or the assertion itself was incorrect: `(off) >= sizeof(arrow)`

PR-URL: #2404
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
The version used before returned -1 on truncation which does not conform
to the standard.

PR-URL: #2404
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
- Added NODE_REPL_HISTORY to the environment variables in the --help
  and made all descriptions start with lower case for consistency.

- Added NODE_REPL_HISTORY and NODE_ICU_DATA to the man page.

PR-URL: #2690
Reviewed-By: fishrock123 - Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: evanlucas - Evan Lucas <evanlucas@me.com>
PR-URL: #2674
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
PR-URL: #2687
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Note: When this was cherry-picked for v8 v4.4 (landed in #2636),
test-heap.cc:1989 chunk did not exist. It now exists in
v8 v4.5.103.30. This PR completes the cherry pick of the whole commit
from v8.

PR-URL: #2692
Reviewed-By: ofrobots - Ali Ijaz Sheikh <ofrobots@google.com>

Original commit message:
  Use static_cast<> for NULL (clang 3.7)

  The following errors come up when compiling v8
   with clang 3.7 on FreeBSD/amd64:

  src/runtime/runtime-i18n.cc:629:37: error: reinterpret_cast from
  'nullptr_t' to 'v8::internal::Smi *' is not allowed
    local_object->SetInternalField(1, reinterpret_cast<Smi*>(NULL));
                                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~

  test/cctest/test-heap.cc:131:20: error: reinterpret_cast from
        'nullptr_t' to 'v8::internal::Object *' is not allowed
    Handle<Object> n(reinterpret_cast<Object*>(NULL), isolate);
                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  test/cctest/test-heap.cc:1989:18: error: reinterpret_cast from
        'nullptr_t' to 'Address' (aka 'unsigned char *') is not
        allowed
    Address base = reinterpret_cast<Address>(NULL);
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

  +add myself to the AUTHORS file.

  BUG=

  Review URL: https://codereview.chromium.org/1277353002

  Cr-Commit-Position: refs/heads/master@{#30103}
There is no type-conversion to be done because both parameters are already the same type. 
Therefore, the === operator should be used for better performance.
@thefourtheye
Copy link
Contributor

Previous effort: #2582

Quoting #2582 (comment),

If it doesn't change the behavior, then it's pointless code churn.

@ChALkeR ChALkeR added the dgram Issues and PRs related to the dgram subsystem / UDP. label Sep 5, 2015
@Trott
Copy link
Member

Trott commented Sep 5, 2015

I could be OK with this one because the file currently contains a mix of == and === for no obvious reason. Making it consistent with itself seems OK. That's not to say I'm +1. More like +0. I wouldn't object to it landing but I wouldn't object to it being closed either.

@Trott
Copy link
Member

Trott commented Sep 7, 2015

I'm going to go ahead and close this because (I assume as a side effect to the fix for #2713) it's jumped from a 1-file change to a 1000-plus file change. If the original submitter (or someone else with appropriate access) wants to do a rebase to get it back to just 1 file, feel free to re-open.

@Trott Trott closed this Sep 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dgram Issues and PRs related to the dgram subsystem / UDP.
Projects
None yet
Development

Successfully merging this pull request may close these issues.