Skip to content

Visual Studio 2013 build warnings #17114

Closed

Description

@d3x0r

Recently updated to node 8.6.0, and when I build my plugin, I get these warnings from node.h.
(when building with cmake-js which does not specify warnings to ignore)
I notice when building with node-gyp that I don't get this warning; but node-gyp adds these ignores to the build. 4267;4351;4355;4800;4251 Although I'm not sure where those come from. I did find (unrelated) this in node-gyp's addon.gypi file. and that hasn't changed since 10/20/2016 so I guess this sort of warning has been an issue on-and-off for a while; although I've had no such warnings around ObjectWrap....

        'msvs_disabled_warnings': [
          # warning C4251: 'node::ObjectWrap::handle_' : class 'v8::Persistent<T>'
          # needs to have dll-interface to be used by
          # clients of class 'node::ObjectWrap'
          4251
        ],

This is a quote of an instance of the warning.

<user>\.cmake-js\node-x64\v8.6.0\include\node\node.h(625): 
   warning C4251: 'node::CallbackScope::try_catch_': class 'v8::TryCatch' needs to have dll-interface to be used by clients of class 'node::CallbackScope' (compiling source file ...]
  <user>\.cmake-js\node-x64\v8.6.0\include\node\v8.h(8150): note: see declaration of 'v8::TryCatch' (compiling source file ...)

Trying to capture what it actually means in order to fix it...

line 8150 of v8.h

class V8_EXPORT TryCatch {

while the class that's using trycatch

class NODE_EXTERN CallbackScope {
 public:
  CallbackScope(v8::Isolate* isolate,
                v8::Local<v8::Object> resource,
                async_context asyncContext);
  ~CallbackScope();

 private:
  InternalCallbackScope* private_;
  v8::TryCatch try_catch_;  // line 625

...

The warning does go away if NODE_EXTERN is removed from the class definition. ObjectWrap has no sort of link specification on it (now). And no other class has NODE_EXTERN applied to it.

In 8.4.0 there was no such thing as 'CallbackScope' so this has been a recent addition, and the extra qualifier has been hidden by node-gyp.


Edit:
I could make this a pull request which is the one line change to node.h; but I think I'd fail because I can run no tests, and can't really get 'affect subsystems'

https://github.com/d3x0r/node/tree/patch-1

Activity

  1. added
    buildIssues and PRs related to Node.js builds or CI infrastructure.
    windowsIssues and PRs related to the Windows platform.
    on Nov 18, 2017
  2. seishun commented on Nov 18, 2017

    @seishun
    Contributor

    It was previously discussed in #15570. Basically, this warning is overzealous. It should hopefully be fixed in VS2017 15.6.

  3. bnoordhuis commented on Nov 24, 2017

    @bnoordhuis
    Member

    Looks like this has been answered. Closing.

  4. d3x0r commented on Dec 7, 2017

    @d3x0r
    ContributorAuthor

    Ya know, this isn't really about node building; this is about node.h causing warnings that node-gyp hides and I would have to hide using cmake-js or other tools.
    the node header previously generated the same C4251 warning with wrapped class, but it was fixed.... this too can be fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    buildIssues and PRs related to Node.js builds or CI infrastructure.windowsIssues and PRs related to the Windows platform.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions