Skip to content

Conversation

alexeykuzmin
Copy link
Contributor

When Node.js is compiled as a part of the Electron build clang generates a couple of errors (see below). Including a header instead of using a forward declaration fixes them.
It would be great if the code could be changed in the Node.js repo so Electron wouldn't have to have a patch for that.

../../vendor/node/src/inspector_agent.cc:516:16: error: member access into incomplete type 'node::NodePlatform'
      platform_->FlushForegroundTasksInternal();
               ^
../../vendor/node/src/inspector_agent.h:19:7: note: forward declaration of 'node::NodePlatform'
class NodePlatform;
      ^
../../vendor/node/src/inspector_agent.cc:670:15: error: assigning to 'v8::Platform *' from incompatible type 'node::NodePlatform *'
  platform_ = platform;
              ^~~~~~~~
2 errors generated.
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

inspector

Include the header instead of using a forward declaration.
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v4.x inspector Issues and PRs related to the V8 inspector protocol labels Nov 2, 2017
@gireeshpunathil
Copy link
Member

@alexeykuzmin
Copy link
Contributor Author

@gireeshpunathil Is it normal that CI jobs are in the pending state for almost 3 days already?

gireeshpunathil pushed a commit that referenced this pull request Nov 6, 2017
Include the header instead of using a forward declaration.

PR-URL: #16677
Reviewed-By: Eugene Ostroukhov <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
@gireeshpunathil
Copy link
Member

@alexeykuzmin - actually the CI has been completed in time, but the view in github is not updated to reflect that fact - probably a bug - @nodejs/collaborators - please share any prior art on this if you know.

Landed in 916e1cb , thanks for the contribution!

@Trott
Copy link
Member

Trott commented Nov 6, 2017

please share any prior art on this if you know.

Yeah, bug in the bot used to update that stuff. I think @maclover7 recently fixed it or was working on fixing it.

@maclover7
Copy link
Contributor

@gireeshpunathil @Trott Build statuses should be reporting properly for all node-test-pull-request builds started after ~noon EST yesterday. If there are PRs you would like to backfill statuses for, I should be able to do that, ping me on IRC :)

cjihrig pushed a commit to cjihrig/node that referenced this pull request Nov 6, 2017
Include the header instead of using a forward declaration.

PR-URL: nodejs#16677
Reviewed-By: Eugene Ostroukhov <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
@cjihrig cjihrig mentioned this pull request Nov 6, 2017
MylesBorins pushed a commit that referenced this pull request Dec 19, 2017
Include the header instead of using a forward declaration.

PR-URL: #16677
Reviewed-By: Eugene Ostroukhov <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
Include the header instead of using a forward declaration.

PR-URL: #16677
Reviewed-By: Eugene Ostroukhov <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants