Skip to content
This repository has been archived by the owner on Feb 17, 2023. It is now read-only.

Include node headers as system headers #37

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

TheMarex
Copy link

@TheMarex TheMarex commented Feb 1, 2018

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

This will avoid problems were GCC/clang might trigger useless warnings induced by including the `v8` header.
TheMarex added a commit to Project-OSRM/osrm-backend that referenced this pull request Feb 1, 2018
This can be removed after cjntaylor/node-cmake#37
is merged and released.
TheMarex added a commit to Project-OSRM/osrm-backend that referenced this pull request Feb 2, 2018
This can be removed after cjntaylor/node-cmake#37
is merged and released.
TheMarex added a commit to Project-OSRM/osrm-backend that referenced this pull request Feb 5, 2018
This can be removed after cjntaylor/node-cmake#37
is merged and released.
TheMarex added a commit to Project-OSRM/osrm-backend that referenced this pull request Feb 5, 2018
This can be removed after cjntaylor/node-cmake#37
is merged and released.
TheMarex added a commit to Project-OSRM/osrm-backend that referenced this pull request Feb 6, 2018
This can be removed after cjntaylor/node-cmake#37
is merged and released.
@cjntaylor
Copy link
Owner

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.

@TheMarex
Copy link
Author

TheMarex commented Feb 7, 2018

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 added a commit to Project-OSRM/osrm-backend that referenced this pull request Feb 9, 2018
This can be removed after cjntaylor/node-cmake#37
is merged and released.
TheMarex added a commit to Project-OSRM/osrm-backend that referenced this pull request Feb 9, 2018
This can be removed after cjntaylor/node-cmake#37
is merged and released.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants