FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

benchmark: use strict mode by Trott · Pull Request #5336 · nodejs/node · GitHub

/ node Public

benchmark: use strict mode - #5336

Closed
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:lint-benchmark
ConversationCommits (1)ChecksFiles changed
Closed

benchmark: use strict mode#5336
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:lint-benchmark

Conversation

Trott commented Feb 20, 2016

Copy link
Copy Markdown
Member

Apply strict mode to benchmark code.

benchmark: use strict mode
0097c12 
Apply strict mode to benchmark code.
Trott added benchmark Issues and PRs related to the benchmark subsystem. lts-watch-v4.x labels Feb 20, 2016

jasnell commented Feb 20, 2016

Copy link
Copy Markdown
Member

@trevnorris @bnoordhuis ... any insight into how this would affect the benchmarks?

silverwind commented Feb 20, 2016

Copy link
Copy Markdown
Contributor

LGTM, I take it this is preparation to lint the benchmarks?

bnoordhuis commented Feb 20, 2016

Copy link
Copy Markdown
Member

LGTM

any insight into how this would affect the benchmarks?

Strict mode should not have a negative impact on the benchmark numbers, if that is what you're asking. It might even improve them.

jasnell commented Feb 20, 2016

Copy link
Copy Markdown
Member

Yep, that's what I was asking! I'm not overly familiar with the internals
of strict mode so wanted to double check. This LGTM
On Feb 20, 2016 3:18 AM, "Ben Noordhuis" notifications@github.com wrote:

LGTM

any insight into how this would affect the benchmarks?

Strict mode should not have a negative impact on the benchmark numbers, if
that is what you're asking. It might even improve them.

—
Reply to this email directly or view it on GitHub
#5336 (comment).

Trott commented Feb 20, 2016

Copy link
Copy Markdown
Member Author

@silverwind asks:

I take it this is preparation to lint the benchmarks?

That's the hope.

evanlucas commented Feb 20, 2016

Copy link
Copy Markdown
Contributor

LGTM

1 similar comment

Fishrock123 commented Feb 21, 2016

Copy link
Copy Markdown
Contributor

LGTM

Fishrock123 reviewed Feb 21, 2016
View reviewed changes
Comment thread benchmark/arrays/var-int.js
@@ -1,3 +1,4 @@
'use strict';

Fishrock123 Feb 21, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

maybe add a space on the 2nd line for these?

Trott Feb 21, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

In our current tests, it is atypical to have a blank line after use strict. (In test/parallel, there are 943 tests that contain use strict. Of those, only 108 contain a blank line after use strict.) So I favored the prevailing convention of no blank line. (Although I see I was not quite 100% consistent with that here. Ugh.)

Anyway, I don't have any strong feelings about it, but that's why I did it the way I did.

evanlucas mentioned this pull request Feb 22, 2016
Closed
Trott added a commit to Trott/io.js that referenced this pull request Feb 22, 2016
benchmark: use strict mode
4bb529d 
Apply strict mode to benchmark code.

PR-URL: nodejs#5336
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Trott commented Feb 22, 2016

Copy link
Copy Markdown
Member Author

Landed in 4bb529d.

It's worth mentioning perhaps that make bench-misc and possibly others were broken before this commit and are still broken, at least on my OS X machine. I'm guessing it's one of:

  • I'm Doing It Wrong
  • it's a known problem among people who do benchmarking
  • people who do benchmarking don't use that and don't care much that it's broken

Trott closed this Feb 22, 2016
rvagg pushed a commit that referenced this pull request Feb 27, 2016
benchmark: use strict mode
2f9fee6 
Apply strict mode to benchmark code.

PR-URL: #5336
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Fishrock123 mentioned this pull request Mar 1, 2016
Merged
5 tasks

MylesBorins commented Mar 17, 2016

Copy link
Copy Markdown
Contributor

@Trott would you be willing to backport this to v4.x?

Trott commented Mar 17, 2016

Copy link
Copy Markdown
Member Author

@thealphanerd If you can get #5123 backported first, that will make this one go a little easier.

MylesBorins commented Mar 17, 2016

Copy link
Copy Markdown
Contributor

@Trott the path improvements are not likely to land soon... if ever

Trott commented Mar 17, 2016

Copy link
Copy Markdown
Member Author

OK, I'll backport this along with the others then.

Trott mentioned this pull request Mar 18, 2016
Closed
Trott deleted the lint-benchmark branch January 13, 2022 22:42
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

benchmark Issues and PRs related to the benchmark subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL