doc: remove x86 from os.arch() options by gibfahn · Pull Request #17899 · nodejs/node

gibfahn · 2017-12-28T11:44:26Z

I don't think it's possible for process.arch (which comes from
V8's target_arch) to be x86.

Also updates process.arch to have the same information.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

YurySolovyov · 2017-12-28T15:06:37Z

Seems like building it like that is still possible

node/configure

Lines 63 to 64 in 9c00f07

    
           valid_arch = ('arm', 'arm64', 'ia32', 'mips', 'mipsel', 'mips64el', 'ppc', 
        
                         'ppc64', 'x32','x64', 'x86', 's390', 's390x')

bnoordhuis

@YurySolovyov It's a synonym for ia32:

node/configure

Lines 862 to 863 in 9c00f07

    
           if target_arch == 'x86': 
        
             target_arch = 'ia32'

bnoordhuis · 2017-12-28T15:53:13Z

doc/api/process.md

The emphasis seems a bit too much but I see os.md does that too.

I would lean towards removing the emphasis, it doesn't really add much. I'm sure @Trott would also have an opinion on this... 😉

apapirovski · 2017-12-28T16:24:25Z

doc/api/process.md

I would lean towards removing the emphasis, it doesn't really add much. I'm sure @Trott would also have an opinion on this... 😉

cjihrig

LGTM. I'm also in favor of removing the emphasis in the paragraph in process.md.

TimothyGu

What is the difference between ia32 and x32?

bnoordhuis · 2017-12-29T00:30:03Z

@TimothyGu https://en.wikipedia.org/wiki/X32_ABI

allows programs to take advantage of the benefits of x86-64 instruction set [...] while using 32-bit pointers and thus avoiding the overhead of 64-bit pointers

gibfahn · 2017-12-29T09:06:37Z

@bnoordhuis so does that mean that an x32 binary wouldn't be able to run on a true Intel 32-bit machine (because it needs the 64-bit instruction set)?

TimothyGu · 2017-12-29T09:27:57Z

I knew something like that existed but forgot the name… oh well, I blame the Windows-esque “x64” terminology.

@gibfahn That’s correct. It provides access to the x86-64 expanded set of registers. I don’t think many people are using it though.

gibfahn · 2018-01-03T22:59:42Z

CI: https://ci.nodejs.org/job/node-test-commit/15215/

It is not possible for `process.arch` (which comes from V8's `target_arch`) to be `x86`. Also updates `process.arch` to have the same information. PR-URL: nodejs#17899 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>

gibfahn · 2018-01-03T23:33:50Z

Landed in ecf6e79

It is not possible for `process.arch` (which comes from V8's `target_arch`) to be `x86`. Also updates `process.arch` to have the same information. PR-URL: #17899 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>

MylesBorins · 2018-01-24T08:57:19Z

Should this land in LTS? It lands cleanly on 6.x and 8.x

gibfahn · 2018-01-24T09:28:36Z

Should this land in LTS? It lands cleanly on 6.x and 8.x

Yes!

It is not possible for `process.arch` (which comes from V8's `target_arch`) to be `x86`. Also updates `process.arch` to have the same information. PR-URL: #17899 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Dec 28, 2017

bnoordhuis approved these changes Dec 28, 2017

View reviewed changes

lpinca approved these changes Dec 28, 2017

View reviewed changes

apapirovski approved these changes Dec 28, 2017

View reviewed changes

jasnell approved these changes Dec 28, 2017

View reviewed changes

cjihrig approved these changes Dec 28, 2017

View reviewed changes

TimothyGu approved these changes Dec 29, 2017

View reviewed changes

danbev approved these changes Dec 29, 2017

View reviewed changes

gibfahn self-assigned this Dec 29, 2017

gibfahn force-pushed the process-arch branch from a9bea3e to ecf6e79 Compare January 3, 2018 23:32

gibfahn merged commit ecf6e79 into nodejs:master Jan 3, 2018

gibfahn deleted the process-arch branch January 3, 2018 23:34

gibfahn mentioned this pull request Jan 4, 2018

Detect merged/closed PR nodejs/node-core-utils#137

Closed

MylesBorins mentioned this pull request Jan 10, 2018

v9.4.0 proposal #18069

Merged

MylesBorins added lts-watch-v6.x labels Jan 24, 2018

MylesBorins added land-on-v6.x and removed lts-watch-v6.x labels Jan 24, 2018

MylesBorins mentioned this pull request Feb 7, 2018

v6.13.0 proposal #18342

Merged

Uh oh!

Conversation

gibfahn commented Dec 28, 2017

Checklist

Affected core subsystem(s)

Uh oh!

YurySolovyov commented Dec 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Dec 28, 2017

Choose a reason for hiding this comment

Uh oh!

apapirovski Dec 28, 2017

Choose a reason for hiding this comment

Uh oh!

gibfahn Dec 28, 2017

Choose a reason for hiding this comment

Uh oh!

apapirovski Dec 28, 2017

Choose a reason for hiding this comment

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

TimothyGu left a comment

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Dec 29, 2017

Uh oh!

gibfahn commented Dec 29, 2017

Uh oh!

TimothyGu commented Dec 29, 2017

Uh oh!

gibfahn commented Jan 3, 2018

Uh oh!

gibfahn commented Jan 3, 2018

Uh oh!

MylesBorins commented Jan 24, 2018

Uh oh!

gibfahn commented Jan 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

YurySolovyov commented Dec 28, 2017 •

edited

Loading