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: do not include x.h if x-inl.h is included #16548

Closed
wants to merge 2 commits into from
Closed

src: do not include x.h if x-inl.h is included #16548

wants to merge 2 commits into from

Conversation

joyeecheung
Copy link
Member

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

src

Refs: #16519

Looks like V8 does not do this anymore: https://github.com/nodejs/node/blob/master/deps/v8/src/objects.cc#L12

So remove the headers. FWIW, less includes to sort, also makes the churn smaller if we want to use clang-format.

Also add this rule to the C++ style guide.

This could use a pre-backport, I can open those if this gets landed (this patch is created with a script anyway).

cc @bnoordhuis

@nodejs-github-bot nodejs-github-bot 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. labels Oct 27, 2017
@joyeecheung
Copy link
Member Author

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

@gibfahn
Copy link
Member

gibfahn commented Oct 27, 2017 •
edited
Loading

This could use a pre-backport, I can open those if this gets landed (this patch is created with a script anyway).

So backport to v6.x and v8.s?

Some things seem to still have the -inl.h version included in the V8 file you linked to:

node/deps/v8/src/objects.cc

Lines 30 to 31 in 363091d

#include "src/counters-inl.h"
#include "src/counters.h"

node/deps/v8/src/objects.cc

Lines 38 to 39 in 363091d

#include "src/field-index-inl.h"
#include "src/field-index.h"

Is this basically just a stylistic change? I guess we previously included both for clarity.

@joyeecheung
Copy link
Member Author

@gibfahn See #16519 (comment) , my guess is the "src/counters.h", .etc are there because they are forgotten, or it's not really a strict rule to delete them.

@gibfahn
Copy link
Member

gibfahn commented Oct 27, 2017

@gibfahn See #16519 (comment) , my guess is the "src/counters.h", .etc are there because they are forgotten, or it's not really a strict rule to delete them.

Ahh, missed that one.

FWIW I'd put this as Fixes: #16519 rather than Refs:, personal preference though.

@joyeecheung joyeecheung mentioned this pull request Oct 30, 2017
Closed
4 tasks
@joyeecheung
Copy link
Member Author

Going to land this once I have opened the two pre-backport.

joyeecheung added a commit to joyeecheung/node that referenced this pull request Oct 30, 2017
@joyeecheung
src: do not include x.h if x-inl.h is included
87a379c 
Fixes: nodejs#16519
PR-URL: nodejs#16548
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
This was referenced Oct 30, 2017
Closed
Closed
@joyeecheung
Copy link
Member Author

joyeecheung commented Oct 30, 2017 •
edited
Loading

Landed in 23a3911...ab2c351, thanks!

joyeecheung added a commit that referenced this pull request Oct 30, 2017
@joyeecheung
src: do not include x.h if x-inl.h is included
cdb263d 
PR-URL: #16548
Fixes: #16519
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
joyeecheung added a commit that referenced this pull request Oct 30, 2017
@joyeecheung
doc: add *-inl.h include rule to C++ style guide
ab2c351 
PR-URL: #16548
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@seishun seishun mentioned this pull request Oct 30, 2017
Closed
2 tasks
gibfahn pushed a commit that referenced this pull request Oct 30, 2017
@joyeecheung @gibfahn
doc: add *-inl.h include rule to C++ style guide
fc8acc8 
PR-URL: #16548
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@joyeecheung joyeecheung mentioned this pull request Oct 31, 2017
Closed
4 tasks
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
@joyeecheung @Qard
src: do not include x.h if x-inl.h is included
b99b330 
PR-URL: nodejs/node#16548
Fixes: nodejs/node#16519
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
@joyeecheung @Qard
doc: add *-inl.h include rule to C++ style guide
1242c66 
PR-URL: nodejs/node#16548
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
@joyeecheung @Qard
src: do not include x.h if x-inl.h is included
35d9c11 
PR-URL: nodejs/node#16548
Fixes: nodejs/node#16519
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
@joyeecheung @Qard
doc: add *-inl.h include rule to C++ style guide
165f62b 
PR-URL: nodejs/node#16548
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
joyeecheung added a commit to joyeecheung/node that referenced this pull request Nov 2, 2017
@joyeecheung
src: do not include x.h if x-inl.h is included
f6a52a2 
Fixes: nodejs#16519
PR-URL: nodejs#16548
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
joyeecheung added a commit to joyeecheung/node that referenced this pull request Nov 2, 2017
@joyeecheung
doc: add *-inl.h include rule to C++ style guide
91a2daa 
PR-URL: nodejs#16548
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
cjihrig pushed a commit to cjihrig/node that referenced this pull request Nov 6, 2017
@joyeecheung @cjihrig
src: do not include x.h if x-inl.h is included
97fd6df 
PR-URL: nodejs#16548
Fixes: nodejs#16519
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
cjihrig pushed a commit to cjihrig/node that referenced this pull request Nov 6, 2017
@joyeecheung @cjihrig
doc: add *-inl.h include rule to C++ style guide
d733dd9 
PR-URL: nodejs#16548
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@cjihrig cjihrig mentioned this pull request Nov 6, 2017
Merged
gibfahn pushed a commit that referenced this pull request Nov 14, 2017
@joyeecheung @gibfahn
src: do not include x.h if x-inl.h is included
255fffb 
Fixes: #16519
PR-URL: #16548
Backport-PR-URL: #16609
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
gibfahn pushed a commit that referenced this pull request Nov 14, 2017
@joyeecheung @gibfahn
doc: add *-inl.h include rule to C++ style guide
a2fd9a3 
PR-URL: #16548
Backport-PR-URL: #16609
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 16, 2017
@joyeecheung @MylesBorins
src: do not include x.h if x-inl.h is included
f20bf69 
Backport-PR-URL: #16610
Fixes: #16519
PR-URL: #16548
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Nov 21, 2017
Merged
@gibfahn gibfahn mentioned this pull request Nov 21, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Nov 21, 2017
@joyeecheung @MylesBorins
src: do not include x.h if x-inl.h is included
9b736f9 
Backport-PR-URL: #16610
Fixes: #16519
PR-URL: #16548
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 28, 2017
@joyeecheung @MylesBorins
src: do not include x.h if x-inl.h is included
b00ced5 
Backport-PR-URL: #16610
Fixes: #16519
PR-URL: #16548
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
@joyeecheung @addaleax
src: do not include x.h if x-inl.h is included
dd9a031 
PR-URL: nodejs/node#16548
Fixes: nodejs/node#16519
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
@joyeecheung @addaleax
doc: add *-inl.h include rule to C++ style guide
94591ed 
PR-URL: nodejs/node#16548
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@danbev danbev danbev approved these changes

@cjihrig cjihrig cjihrig approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil 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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@joyeecheung @gibfahn @bnoordhuis @danbev @cjihrig @gireeshpunathil @MylesBorins @nodejs-github-bot