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

src: rename async-wrap -> async_wrap #17022

Closed
wants to merge 3 commits into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Nov 14, 2017

This commit renames async-wrap to async_wrap for consitency with other
c++ source files.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

src

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Nov 14, 2017
@danbev
Copy link
Contributor Author

danbev commented Nov 14, 2017

@refack
Copy link
Contributor

refack commented Nov 14, 2017

@danbev is there a good motivation (even something like a simpler glob in some script)?

@apapirovski
Copy link
Member

apapirovski commented Nov 14, 2017

@refack I don't see why not, the current naming is pretty likely to lead to typos during development. (Speaking from personal experience with ReqWrap...)

(I don't think this affects backporting or anything, does it?)

@danbev
Copy link
Contributor Author

danbev commented Nov 14, 2017

@danbev is there a good motivation

The motivation is simply that I've noticed this inconsistency and at one point thought it might mean something that some source files were named differently. As there does not seem to be any reason for this, having them named consistently seems to make sense to me.

@danbev
Copy link
Contributor Author

danbev commented Nov 16, 2017

Any preferences to how these commits should land, squashed as one single commit (and update the commit message to reflect that) or as separate commits?

The only reason for multiple commit would be to make reverting easier if required.

@danbev
Copy link
Contributor Author

danbev commented Nov 16, 2017

@danbev
Copy link
Contributor Author

danbev commented Nov 16, 2017

test/linux-openssl110 failures look unrelated

console output:

not ok 1867 parallel/test-http2-create-client-connect # TODO : Fix flaky test
  ---
  duration_ms: 120.35
  severity: fail
  stack: |-
    timeout
  ...

not ok 101 async-hooks/test-signalwrap
  ---
  duration_ms: 0.415
  severity: fail
  stack: |-
    Mismatched onsigusr2 function calls. Expected exactly 2, actual 1.
        at Object.exports.mustCall (/home/iojs/build/workspace/node-test-commit-linux-linked/nodes/ubuntu1604_sharedlibs_openssl110_x64/test/common/index.js:490:10)
        at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux-linked/nodes/ubuntu1604_sharedlibs_openssl110_x64/test/async-hooks/test-signalwrap.js:14:30)
        at Module._compile (module.js:644:30)
        at Object.Module._extensions..js (module.js:655:10)
        at Module.load (module.js:563:32)
        at tryModuleLoad (module.js:506:12)
        at Function.Module._load (module.js:498:3)
        at Function.Module.runMain (module.js:685:10)
        at startup (bootstrap_node.js:192:16)
  ...

This commit renames async-wrap to async_wrap for consitency with other
c++ source files.
This commit renames base-object to base_object for consitency with other
c++ source files.
This commit renames req-wrap to req_wrap consitency with other
c++ source files.
@danbev
Copy link
Contributor Author

danbev commented Nov 17, 2017

Rebased CI: https://ci.nodejs.org/job/node-test-pull-request/11494/
I'll try to land this if there are no surprises in the CI results.

@danbev
Copy link
Contributor Author

danbev commented Nov 17, 2017

test/linux-debug failures look unrelated

console output:

not ok 499 parallel/test-error-reporting # TODO : Fix flaky test
  ---
  duration_ms: 70.88
  severity: flaky
  stack: |-
    assert.js:42
      throw new errors.AssertionError({
      ^
    
    AssertionError [ERR_ASSERTION]: false == true
        at /home/iojs/build/workspace/node-test-commit-linux-linked/nodes/ubuntu1604_sharedlibs_debug_x64/test/parallel/test-error-reporting.js:80:10
        at /home/iojs/build/workspace/node-test-commit-linux-linked/nodes/ubuntu1604_sharedlibs_debug_x64/test/common/index.js:522:15
        at /home/iojs/build/workspace/node-test-commit-linux-linked/nodes/ubuntu1604_sharedlibs_debug_x64/test/parallel/test-error-reporting.js:38:5
        at ChildProcess.exithandler (child_process.js:279:5)
        at ChildProcess.emit (events.js:159:13)
        at maybeClose (internal/child_process.js:943:16)
        at Process.ChildProcess._handle.onexit (internal/child_process.js:220:5)
  ...
not ok 789 parallel/test-http-pipeline-flood
  ---
  duration_ms: 58.41
  severity: fail
  stack: |-
    Mismatched <anonymous> function calls. Expected exactly 1, actual 2.
        at Object.exports.mustCall (/home/iojs/build/workspace/node-test-commit-linux-linked/nodes/ubuntu1604_sharedlibs_debug_x64/test/common/index.js:490:10)
        at Server.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux-linked/nodes/ubuntu1604_sharedlibs_debug_x64/test/parallel/test-http-pipeline-flood.js:59:35)
        at Object.onceWrapper (events.js:254:19)
        at Server.emit (events.js:159:13)
        at emitListeningNT (net.js:1394:10)
        at _combinedTickCallback (internal/process/next_tick.js:135:11)
        at process._tickCallback (internal/process/next_tick.js:180:9)
        at Function.Module.runMain (module.js:687:11)
        at startup (bootstrap_node.js:192:16)
not ok 1856 parallel/test-zlib
  ---
  duration_ms: 480.251
  severity: fail
  stack: |-
    timeout
  ...
not ok 1898 sequential/test-inspector-contexts # TODO : Fix flaky test
  ---
  duration_ms: 6.830
  severity: crashed
  stack: |-
    oh no!
    exit code: CRASHED (Signal: 4)
  ...
not ok 1934 sequential/test-repl-timeout-throw
  ---
  duration_ms: 10.517
  severity: fail
  stack: |-
    > ''
    > THROW 0
    ReferenceError: XXX is not defined
        at Timeout.thrower [as _onTimeout] (repl:1:116)
        at ontimeout (timers.js:478:11)
        at tryOnTimeout (timers.js:302:5)
        at Timer.listOnTimeout (timers.js:262:5)
    > undefined
    > ... ... ... ..... ..... ....... ....... ..... ... ''
    > THROW 1
    ReferenceError: XXX is not defined
        at EventEmitter.thrower (repl:1:116)
        at EventEmitter.emit (events.js:159:13)
    > 2
    var throws = 0;process.on("exit",function(){console.log(throws)});function thrower(){console.log("THROW",throws++);XXX};setTimeout(thrower);""
    fs.readFile("/home/iojs/build/workspace/node-test-commit-linux-linked/nodes/ubuntu1604_sharedlibs_debug_x64/test/sequential/test-repl-timeout-throw.js", thrower);
    setTimeout(function() {
      const events = require("events");
      var e = new events.EventEmitter;
      process.nextTick(function() {
        e.on("x", thrower);
        setTimeout(function() {
          e.emit("x");
        });
      });
    });"";
    assert.js:42
      throw new errors.AssertionError({
      ^
    
    AssertionError [ERR_ASSERTION]: '> 2' === '> 3'
        at ChildProcess.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux-linked/nodes/ubuntu1604_sharedlibs_debug_x64/test/sequential/test-repl-timeout-throw.js:58:10)
        at ChildProcess.emit (events.js:159:13)
        at maybeClose (internal/child_process.js:943:16)
        at Process.ChildProcess._handle.onexit (internal/child_process.js:220:5)
  ...

@danbev
Copy link
Contributor Author

danbev commented Nov 17, 2017

Landed in 7844766, a515e59, and b58a1cd

@danbev danbev closed this Nov 17, 2017
danbev added a commit that referenced this pull request Nov 17, 2017
This commit renames async-wrap to async_wrap for consitency with other
c++ source files.

PR-URL: #17022
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
danbev added a commit that referenced this pull request Nov 17, 2017
This commit renames base-object to base_object for consitency with other
c++ source files.

PR-URL: #17022
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
danbev added a commit that referenced this pull request Nov 17, 2017
This commit renames req-wrap to req_wrap consitency with other
c++ source files.

PR-URL: #17022
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@danbev danbev deleted the consistent-naming-sources branch November 17, 2017 12:16
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
This commit renames async-wrap to async_wrap for consitency with other
c++ source files.

PR-URL: #17022
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
This commit renames base-object to base_object for consitency with other
c++ source files.

PR-URL: #17022
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
This commit renames req-wrap to req_wrap consitency with other
c++ source files.

PR-URL: #17022
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
@gibfahn
Copy link
Member

gibfahn commented Dec 19, 2017

@danbev this conflicts on v8.x, would you mind raising a backport (assuming it's just a find-replace).

gibfahn pushed a commit to gibfahn/node that referenced this pull request Jan 17, 2018
This commit renames async-wrap to async_wrap for consitency with other
c++ source files.

PR-URL: nodejs#17022
Backport-PR-URL: nodejs#18179
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MylesBorins pushed a commit that referenced this pull request Jan 19, 2018
This commit renames async-wrap to async_wrap for consitency with other
c++ source files.

Backport-PR-URL: #18179
PR-URL: #17022
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants