node-cmake icon indicating copy to clipboard operation
node-cmake copied to clipboard

Include node headers as system headers

Open TheMarex opened this issue 7 years ago • 2 comments

This will avoid problems were GCC/clang might trigger useless warnings induced by including the v8 header.

TheMarex avatar Feb 01 '18 12:02 TheMarex

So, for a bit of history, you'll need to look at #30

I've been hesitant to change to this because I've long felt that this may end up masking legitimate warnings about improper uses of v8 in your own module. You have to make calls to v8 functions (even if you use NAN, which just abstracts them) in your module code, so I'm not convinced that the headers included here should really be treated as SYSTEM headers.

To say this another way, I haven't yet been convinced that SYSTEM won't mask legitimate warnings that should be fixed when building your module, so this may end up causing more problems than it solves. As best I can tell node-gyp doesn't do this either. If you want this merged, I'll need to be convinced this can't happen.

#30 suggests a method of doing this locally for your own project if you really need this behaviour.

cjntaylor avatar Feb 07 '18 06:02 cjntaylor

To say this another way, I haven't yet been convinced that SYSTEM won't mask legitimate warnings that should be fixed when building your module

It does not mask warnings inside your module, only warnings that occur when parsing the node header file. This is noise and is turned off for all dependency includes in any C++ project. If you are not convinced, please consider that node headers placed in /usr/include would also get the SYSTEM flag by default. The only reason we see this error is that it is a non-default include path.

TheMarex avatar Feb 07 '18 11:02 TheMarex