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

lib: refactor crypto cipher/hash/curve getters #10682

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Jan 8, 2017

  • refactor internal util.filterDuplicateStrings() to eliminate unused
    code paths
  • .indexOf() -> .includes() in test
  • more concise arrow functions
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

crypto util test lib

@Trott Trott added crypto Issues and PRs related to the crypto subsystem. lib / src Issues and PRs related to general changes in the lib or src directory. test Issues and PRs related to the tests. util Issues and PRs related to the built-in util module. labels Jan 8, 2017
@nodejs-github-bot nodejs-github-bot added crypto Issues and PRs related to the crypto subsystem. util Issues and PRs related to the built-in util module. lts-watch-v6.x labels Jan 8, 2017
@Trott
Copy link
Member Author

Trott commented Jan 8, 2017

Benchmark results:

$ node benchmark/compare.js --old ./node-master --new ./node-refactored --filter get-ciphers crypto > compare-crypto.csv
$ cat compare-crypto.csv | Rscript benchmark/compare.R
                                         improvement significant    p.value
 crypto/get-ciphers.js v="crypto" n=1         6.04 %           * 0.02953932
 crypto/get-ciphers.js v="crypto" n=5000     -0.75 %             0.66187606
 crypto/get-ciphers.js v="tls" n=1            7.92 %           * 0.02733195
 crypto/get-ciphers.js v="tls" n=5000         1.18 %             0.48262097
$ 

* refactor internal util.filterDuplicateStrings() to eliminate unused
  code paths
* `.indexOf()` -> `.includes()` in test
* more concise arrow functions
@Trott
Copy link
Member Author

Trott commented Jan 8, 2017

@Trott
Copy link
Member Author

Trott commented Jan 8, 2017

(CI is green.)

jasnell pushed a commit that referenced this pull request Jan 10, 2017
* refactor internal util.filterDuplicateStrings() to eliminate unused
  code paths
* `.indexOf()` -> `.includes()` in test
* more concise arrow functions

PR-URL: #10682
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michal Zasso <targos@protonmail.com>
@jasnell
Copy link
Member

jasnell commented Jan 10, 2017

Landed in 022b53c

@jasnell jasnell closed this Jan 10, 2017
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
* refactor internal util.filterDuplicateStrings() to eliminate unused
  code paths
* `.indexOf()` -> `.includes()` in test
* more concise arrow functions

PR-URL: nodejs#10682
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michal Zasso <targos@protonmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 23, 2017
* refactor internal util.filterDuplicateStrings() to eliminate unused
  code paths
* `.indexOf()` -> `.includes()` in test
* more concise arrow functions

PR-URL: nodejs#10682
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michal Zasso <targos@protonmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 25, 2017
* refactor internal util.filterDuplicateStrings() to eliminate unused
  code paths
* `.indexOf()` -> `.includes()` in test
* more concise arrow functions

PR-URL: nodejs#10682
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michal Zasso <targos@protonmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 27, 2017
* refactor internal util.filterDuplicateStrings() to eliminate unused
  code paths
* `.indexOf()` -> `.includes()` in test
* more concise arrow functions

PR-URL: nodejs#10682
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michal Zasso <targos@protonmail.com>
@italoacasas italoacasas mentioned this pull request Jan 29, 2017
MylesBorins pushed a commit that referenced this pull request Mar 8, 2017
* refactor internal util.filterDuplicateStrings() to eliminate unused
  code paths
* `.indexOf()` -> `.includes()` in test
* more concise arrow functions

PR-URL: #10682
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michal Zasso <targos@protonmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
* refactor internal util.filterDuplicateStrings() to eliminate unused
  code paths
* `.indexOf()` -> `.includes()` in test
* more concise arrow functions

PR-URL: #10682
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michal Zasso <targos@protonmail.com>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
@Trott Trott deleted the crypto-refactor branch January 13, 2022 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem. lib / src Issues and PRs related to general changes in the lib or src directory. test Issues and PRs related to the tests. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants