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.

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

src: organize imports in inspector_profiler.cc #29073

Closed
wants to merge 1 commit into from

Conversation

@pi1024e
Copy link
Contributor

commented Aug 9, 2019

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

In the other .cc files in the project, includes are in alphabetical order, with local files first, and libraries after. However, inspector_profiler.cc has a library declared in the middle of the import order, and v8 is the second to last being imported, instead of the last. So I reordered the imports and testing showed no side effects; everything passed.

It is a small change and it does not change any behavior. However, if you are weary of accepting this PR I understand.

@nodejs-github-bot

This comment has been minimized.

@pi1024e pi1024e force-pushed the pi1024e:master branch 2 times, most recently from 1eef586 to 0f75d50 Aug 9, 2019

@eugeneo
eugeneo approved these changes Aug 9, 2019
@nodejs-github-bot

This comment has been minimized.

@Trott
Trott approved these changes Aug 9, 2019
@nodejs-github-bot

This comment has been minimized.

@pi1024e pi1024e force-pushed the pi1024e:master branch 2 times, most recently from a395d7e to b688058 Aug 10, 2019

@pi1024e pi1024e changed the title src: organize imports in inspector_profiler.cc to match import order of other files src: organize imports in inspector_profiler.cc Aug 10, 2019

@pi1024e pi1024e force-pushed the pi1024e:master branch from 9f78fca to f0fdb47 Aug 10, 2019

@ZYSzys
ZYSzys approved these changes Aug 10, 2019

@pi1024e pi1024e force-pushed the pi1024e:master branch from f0fdb47 to 85c0575 Aug 11, 2019

@pi1024e pi1024e force-pushed the pi1024e:master branch from 12d4dbd to fb173ae Aug 11, 2019

@nodejs-github-bot

This comment has been minimized.

@Trott

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

Landed in 15b2d13.

Thanks for the contribution! 🎉

@Trott Trott closed this Aug 12, 2019

Trott added a commit to Trott/io.js that referenced this pull request Aug 12, 2019
src: organize imports in inspector_profiler.cc
In the other .cc files in the project, includes are in alphabetical
order, with local files first, and libraries after. However,
inspector_profiler.cc has a library declared in the middle of the import
order, and v8 is the second to last being imported, instead of the last.
So I reordered the imports and testing showed no side effects;
everything passed.

PR-URL: nodejs#29073
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
targos added a commit that referenced this pull request Aug 19, 2019
src: organize imports in inspector_profiler.cc
In the other .cc files in the project, includes are in alphabetical
order, with local files first, and libraries after. However,
inspector_profiler.cc has a library declared in the middle of the import
order, and v8 is the second to last being imported, instead of the last.
So I reordered the imports and testing showed no side effects;
everything passed.

PR-URL: #29073
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
targos added a commit that referenced this pull request Aug 19, 2019
src: organize imports in inspector_profiler.cc
In the other .cc files in the project, includes are in alphabetical
order, with local files first, and libraries after. However,
inspector_profiler.cc has a library declared in the middle of the import
order, and v8 is the second to last being imported, instead of the last.
So I reordered the imports and testing showed no side effects;
everything passed.

PR-URL: #29073
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
@targos targos referenced this pull request Aug 19, 2019
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
src: organize imports in inspector_profiler.cc
In the other .cc files in the project, includes are in alphabetical
order, with local files first, and libraries after. However,
inspector_profiler.cc has a library declared in the middle of the import
order, and v8 is the second to last being imported, instead of the last.
So I reordered the imports and testing showed no side effects;
everything passed.

PR-URL: nodejs#29073
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
src: organize imports in inspector_profiler.cc
In the other .cc files in the project, includes are in alphabetical
order, with local files first, and libraries after. However,
inspector_profiler.cc has a library declared in the middle of the import
order, and v8 is the second to last being imported, instead of the last.
So I reordered the imports and testing showed no side effects;
everything passed.

PR-URL: nodejs#29073
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.