GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — src: re-sort the symbol macros by sam-github · Pull Request #24382 · nodejs/node

Conversation

@sam-github

@sam-github sam-github commented Nov 15, 2018

Copy link
Copy Markdown
Contributor

The symbol macros were almost lexically sorted, but some were misplaced.

I noticed when I added a few, then sorted the symbol block expecting the new ones to just go into the right place, which they did, but a dozen pre-existing ones moved around, too.

I think this could be fast-tracked, it should make no functional difference.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@nodejs-github-bot

nodejs-github-bot commented Nov 15, 2018

Copy link
Copy Markdown
Collaborator

@sam-github build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1624/pipeline
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Nov 15, 2018
@sam-github

sam-github commented Nov 15, 2018

Copy link
Copy Markdown
Contributor Author

I don't see how this would need anything more than a ci lite to land, and perhaps it is a candidate for fast tracing? Not sure if we fast track based on triviality, or urgency.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
devsnek
devsnek approved these changes Nov 15, 2018
View reviewed changes
@devsnek

devsnek commented Nov 15, 2018

Copy link
Copy Markdown
Member

code changes should always get the full ci. i have no opinion about the fast tracking beyond that
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
refack
refack approved these changes Nov 15, 2018
View reviewed changes
@refack

refack commented Nov 15, 2018

Copy link
Copy Markdown
Contributor

urgency

Urgency yes (regression and red CI fixers), and if the possible impact is minimal. Personally I'm not a big fan of fast tracking, but I won't block this on.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@sam-github

sam-github commented Nov 15, 2018

Copy link
Copy Markdown
Contributor Author

ci: https://ci.nodejs.org/job/node-test-pull-request/18660/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
cjihrig
cjihrig approved these changes Nov 15, 2018
View reviewed changes
fhinkel
fhinkel approved these changes Nov 16, 2018
View reviewed changes
addaleax
addaleax approved these changes Nov 16, 2018
View reviewed changes
@addaleax

addaleax commented Nov 16, 2018

Copy link
Copy Markdown
Member

I’d be in favour of fast-tracking, since this PR is not complex and there’s a decent chance of generating merge conflicts that we could otherwise avoid.
cjihrig reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@addaleax addaleax added the fast-track PRs proposed for a shorter-than-standard waiting period before landing. label Nov 16, 2018
@refack refack added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Nov 16, 2018
@sam-github @refack
src: re-sort the symbol macros
5f9b624 
The symbol macros were almost lexically sorted, but some were misplaced.

PR-URL: nodejs#24382
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@refack
refack force-pushed the sort-env-symbols branch from ffcd730 to 5f9b624 Compare November 16, 2018 14:17
@refack

refack commented Nov 16, 2018

Copy link
Copy Markdown
Contributor

Pre-land extra CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/1631/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@refack
refack merged commit 5f9b624 into nodejs:master Nov 16, 2018
targos pushed a commit that referenced this pull request Nov 18, 2018
@sam-github @targos
src: re-sort the symbol macros
459e144 
The symbol macros were almost lexically sorted, but some were misplaced.

PR-URL: #24382
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@joyeecheung joyeecheung mentioned this pull request Nov 19, 2018
src: use STL containers instead of v8 values for static module data #24384
Closed
4 tasks done
@sam-github
sam-github deleted the sort-env-symbols branch November 27, 2018 22:00
rvagg pushed a commit that referenced this pull request Nov 28, 2018
@sam-github @rvagg
src: re-sort the symbol macros
fa9e03c 
The symbol macros were almost lexically sorted, but some were misplaced.

PR-URL: #24382
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Dec 5, 2018
v11.4.0 proposal #24854
Merged
4 tasks done
This was referenced Dec 7, 2018
2018-12-07 Version 11.4.0 (Current) @BridgeAR beevelop/docker-android-nodejs#261
Closed
2018-12-07 Version 11.4.0 (Current) @BridgeAR beevelop/docker-android-nodejs#262
Closed
2018-12-07 Version 11.4.0 (Current) @BridgeAR beevelop/docker-android-nodejs#263
Closed
2018-12-07 Version 11.4.0 (Current) @BridgeAR beevelop/docker-android-nodejs#264
Closed
2018-12-07 Version 11.4.0 (Current) @BridgeAR beevelop/docker-android-nodejs#265
Closed
@codebytere

codebytere commented Jan 12, 2019

Copy link
Copy Markdown
Member

@sam-github do you think this is worth trying to backport to v10.x? it's one huge conflict, unfortunately
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@codebytere codebytere added the backport-requested-v10.x label Jan 12, 2019
@sam-github

sam-github commented Jan 14, 2019

Copy link
Copy Markdown
Contributor Author

Yes, I'll do it, because PRs on master that touch the sorted macros won't merge clean unless the 10.x macros are also sorted. Its trivial to backport, I'll just resort the macros that exist already in 10.x.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@sam-github sam-github mentioned this pull request Jan 14, 2019
[v10.x backport] src: re-sort the symbol macros #25500
Closed
4 tasks
@sam-github

sam-github commented Jan 14, 2019

Copy link
Copy Markdown
Contributor Author

v10.x backport: #25500
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
sam-github added a commit to sam-github/node that referenced this pull request Feb 4, 2019
@sam-github
src: re-sort the symbol macros
c76c3c9 
The symbol macros were almost lexically sorted, but some were misplaced.

Backport-PR-URL: nodejs#25500
PR-URL: nodejs#24382
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 5, 2019
@sam-github @BethGriggs
src: re-sort the symbol macros
26f7edb 
The symbol macros were almost lexically sorted, but some were misplaced.

Backport-PR-URL: #25500
PR-URL: #24382
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Feb 12, 2019
v10.15.3 proposal #26063
Merged
rvagg pushed a commit that referenced this pull request Feb 28, 2019
@sam-github @rvagg
src: re-sort the symbol macros
46f5c32 
The symbol macros were almost lexically sorted, but some were misplaced.

Backport-PR-URL: #25500
PR-URL: #24382
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@sam-github sam-github added backported-to-v10.x and removed backport-requested-v10.x labels Apr 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleax addaleax addaleax approved these changes
@cjihrig cjihrig cjihrig approved these changes
+3 more reviewers
@refack refack refack approved these changes
@fhinkel fhinkel fhinkel approved these changes
@devsnek devsnek devsnek approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. c++ Issues and PRs that require attention from people who are familiar with C++. fast-track PRs proposed for a shorter-than-standard waiting period before landing.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@sam-github @nodejs-github-bot @devsnek @refack @addaleax @codebytere @fhinkel @cjihrig
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.