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

Why do we include both *-inl.h and *.h in the same .cc file? #16519

Closed
joyeecheung opened this issue Oct 26, 2017 · 2 comments
Closed

Why do we include both *-inl.h and *.h in the same .cc file? #16519

joyeecheung opened this issue Oct 26, 2017 · 2 comments
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. question Issues that look for answers.

Comments

@joyeecheung
Copy link
Member

joyeecheung commented Oct 26, 2017 •
edited
Loading

  • Version: master
  • Subsystem: src

Just noticed this when doing #16122, clang-format has a different idea of alphabetical orders and put -inl.h includes before .h includes. Is there a reason that we include them both? Usually including -inl.h is enough because they should include .h as well, and they should be self-contained.
@joyeecheung joyeecheung added lib / src Issues and PRs related to general changes in the lib or src directory. question Issues that look for answers. c++ Issues and PRs that require attention from people who are familiar with C++. labels Oct 26, 2017
@joyeecheung
Copy link
Member Author

joyeecheung commented Oct 26, 2017 •
edited
Loading

Also if there is a reason for that I think it's worth adding that to the C++ style guide, AFAICT this is different from how V8 includes stuff as well.

@bnoordhuis
Copy link
Member

No real reason. I think I introduced it in the node.js code base and I probably copied it from the V8 code base.

@joyeecheung joyeecheung mentioned this issue Oct 27, 2017
Closed
3 tasks
joyeecheung added a commit to joyeecheung/node that referenced this issue 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]>
gibfahn pushed a commit that referenced this issue Oct 30, 2017
@joyeecheung @gibfahn
src: do not include x.h if x-inl.h is included
39ed0fe 
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]>
gibfahn pushed a commit that referenced this issue Oct 30, 2017
@joyeecheung @gibfahn
src: do not include x.h if x-inl.h is included
13c03ff 
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]>
Qard pushed a commit to ayojs/ayo that referenced this issue 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 issue 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]>
joyeecheung added a commit to joyeecheung/node that referenced this issue 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]>
cjihrig pushed a commit to cjihrig/node that referenced this issue 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]>
gibfahn pushed a commit that referenced this issue 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]>
MylesBorins pushed a commit that referenced this issue 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 pushed a commit that referenced this issue 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 issue 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 issue 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. question Issues that look for answers.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@bnoordhuis @joyeecheung