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.

Sign up for GitHub

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

Jump to bottom

src: add fcntl.h include to node.cc #12540

Closed
wants to merge 1 commit into from
Closed

src: add fcntl.h include to node.cc #12540

wants to merge 1 commit into from

Conversation

bzoz
Copy link
Contributor

@bzoz bzoz commented Apr 20, 2017

PR #11863 added _O_RDWR to node.cc which is defined in fcntl.h. This adds this include directly to node.cc to prevent build fails like in #12498

/cc @MylesBorins

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

src

@bzoz
src: add fcntl.h include to node.cc
8c34036 
nodejs#11863 adds _O_RDWR to node.cc
which is defined in fcntl.h. This adds this include directly to
node.cc.
@bzoz bzoz 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. lts-watch-v4.x process Issues and PRs related to the process subsystem. labels Apr 20, 2017
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Apr 20, 2017
@bzoz bzoz mentioned this pull request Apr 20, 2017
Merged
@gibfahn
Copy link
Member

gibfahn commented Apr 21, 2017

cc/ @nodejs/platform-windows

@MylesBorins
Copy link
Contributor

ci: https://ci.nodejs.org/job/node-test-pull-request/7625/

TimothyGu
TimothyGu reviewed Apr 24, 2017
View reviewed changes
src/node.cc
@@ -80,6 +80,7 @@
#include <stdlib.h>
#include <string.h>
#include <sys/types.h>
#include <fcntl.h>
Copy link
Member

Choose a reason for hiding this comment

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

Can you please maintain the alphabetical order? A comment detailing what macro is used like above for PATH_MAX would be helpful too.

@refack refack mentioned this pull request Apr 24, 2017
Closed
3 tasks
MylesBorins
MylesBorins approved these changes Apr 24, 2017
View reviewed changes
Copy link
Contributor

@MylesBorins MylesBorins left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM

@MylesBorins
Copy link
Contributor

Landed in 43c1b62
I took the liberty to alphabetize and include a comment as per @TimothyGu's request

MylesBorins pushed a commit that referenced this pull request Apr 24, 2017
@bzoz @MylesBorins
src: add fcntl.h include to node.cc
43c1b62 
#11863 adds _O_RDWR to node.cc
which is defined in fcntl.h. This adds this include directly to
node.cc.

PR-URL: #12540
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
@eugeneo
Copy link
Contributor

eugeneo commented Apr 24, 2017

Commit 43c1b62 breaks cpp lint - there's an unwanted dot

@eugeneo
Copy link
Contributor

eugeneo commented Apr 24, 2017

9b4ae9f has the dot as well :)

@MylesBorins
Copy link
Contributor

@eugeneo BAHHHH #12626

@jasnell
Copy link
Member

jasnell commented Apr 24, 2017

I totally missed the dot also lol ... linting didn't complain about that one.

MylesBorins pushed a commit that referenced this pull request Apr 24, 2017
@bzoz @MylesBorins
src: add fcntl.h include to node.cc
58230b4 
#11863 adds _O_RDWR to node.cc
which is defined in fcntl.h. This adds this include directly to
node.cc.

PR-URL: #12540
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 24, 2017
@bzoz @MylesBorins
src: add fcntl.h include to node.cc
fdb8975 
#11863 adds _O_RDWR to node.cc
which is defined in fcntl.h. This adds this include directly to
node.cc.

PR-URL: #12540
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
@refack refack mentioned this pull request Apr 24, 2017
Closed
MylesBorins pushed a commit that referenced this pull request Apr 24, 2017
@bzoz @MylesBorins
src: add fcntl.h include to node.cc
c667e6e 
#11863 adds _O_RDWR to node.cc
which is defined in fcntl.h. This adds this include directly to
node.cc.

PR-URL: #12540
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 24, 2017
@bzoz @MylesBorins
src: add fcntl.h include to node.cc
117195a 
#11863 adds _O_RDWR to node.cc
which is defined in fcntl.h. This adds this include directly to
node.cc.

PR-URL: #12540
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 24, 2017
@bzoz @MylesBorins
src: add fcntl.h include to node.cc
b5b78b1 
#11863 adds _O_RDWR to node.cc
which is defined in fcntl.h. This adds this include directly to
node.cc.

PR-URL: #12540
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
@refack refack mentioned this pull request Apr 25, 2017
Closed
evanlucas pushed a commit that referenced this pull request Apr 25, 2017
@bzoz @evanlucas
src: add fcntl.h include to node.cc
c02e12b 
#11863 adds _O_RDWR to node.cc
which is defined in fcntl.h. This adds this include directly to
node.cc.

PR-URL: #12540
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
@evanlucas evanlucas mentioned this pull request May 1, 2017
Merged
evanlucas pushed a commit that referenced this pull request May 1, 2017
@bzoz @evanlucas
src: add fcntl.h include to node.cc
06db34b 
#11863 adds _O_RDWR to node.cc
which is defined in fcntl.h. This adds this include directly to
node.cc.

PR-URL: #12540
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
@MylesBorins MylesBorins mentioned this pull request May 2, 2017
Merged
evanlucas pushed a commit that referenced this pull request May 2, 2017
@bzoz @evanlucas
src: add fcntl.h include to node.cc
520f876 
#11863 adds _O_RDWR to node.cc
which is defined in fcntl.h. This adds this include directly to
node.cc.

PR-URL: #12540
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
@bzoz @andrew749
src: add fcntl.h include to node.cc
d06b69b 
nodejs/node#11863 adds _O_RDWR to node.cc
which is defined in fcntl.h. This adds this include directly to
node.cc.

PR-URL: nodejs/node#12540
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TimothyGu TimothyGu TimothyGu left review comments

@refack refack refack approved these changes

@jasnell jasnell jasnell approved these changes

@MylesBorins MylesBorins MylesBorins approved these changes

@addaleax addaleax addaleax approved these changes

@cjihrig cjihrig cjihrig approved these changes

@richardlau richardlau richardlau approved these changes

@gibfahn gibfahn gibfahn approved these changes

Assignees
No one assigned
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. process Issues and PRs related to the process subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.