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

build: fix or suppress warnings #8485

Closed
wants to merge 4 commits into from
Closed

build: fix or suppress warnings #8485

wants to merge 4 commits into from

Conversation

refack
Copy link
Contributor

@refack refack commented Oct 2, 2014

  • activated "treat warnings as errors"

@trevnorris
Copy link

/cc @orangemocha @indutny

indutny
indutny reviewed Oct 7, 2014
View reviewed changes
common.gypi
'VCLibrarianTool': {
'AdditionalOptions': [
'/ignore:4221', # link time code generation
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense.

@refack
Copy link
Contributor Author

refack commented Oct 8, 2014

P.S. this is the output on compiling the node target with all warnings enabled:
https://gist.github.com/refack/65eb7b126e1c9b697134

@refack
Copy link
Contributor Author

refack commented Oct 8, 2014

Refactored the whole thing.

indutny
indutny reviewed Oct 8, 2014
View reviewed changes
.gitignore


#Jetbrains project files (WebStorm / Clion)
/.idea/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing newline.

@indutny
Copy link
Member

indutny commented Oct 8, 2014

Some nits, but mostly LGTM

@refack
Copy link
Contributor Author

refack commented Oct 8, 2014

Dropped the whole style fix commit.
I still need convincing about 'WarnAsError'. Again it's MSVS only, and it works. The compiler is our friend why not use it. If someone introduces a new warning to the windows build someone must look at it.
The logic behind 'cflags!': ['-Werror'] is explained in the comment, but here it's a specific compiler suite on a specific OS why let it accumulate warnings, as it has till now.

@refack
Copy link
Contributor Author

refack commented Oct 8, 2014

rebased on #8476

@orangemocha
Copy link
Contributor

Most developers build/test their changes on one platform. Since not everybody builds on Windows, treating warning as errors will cause a lot of build breaks!
I hope that one day we will have a better continuous integration infrastructure that will build/test every PR on all platforms. If we had that, we could turn WarnAsError on, but until then I believe it will bring more trouble than benefit.

orangemocha
orangemocha reviewed Oct 8, 2014
View reviewed changes
common.gypi
# 4267 - int64 passed as int, truncation might happen (depends on linkage)
# 4530 - No exception semantics (leaking from MS STL xlocale)
# 4996 - winsock ip4 calls deprecated
'msvs_disabled_warnings': [4351, 4355, 4800, 4244, 4267, 4530, 4996],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the exception of maybe C4351, I don't think it's a good idea to disable these warnings. And for warnings that cannot be fixed, like the ones about deprecation, it would be better to disable them in the source file using #pragma

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are here for deps sake. Only 4244, 4267 are allowed in node code.

@refack
Copy link
Contributor Author

refack commented Oct 8, 2014

That's like saying if a PR introduces a failing test on some platform that's Ok, as long as the build works.
I believe that build breaks are better than software bugs.
We use a compiler, cpplint, jslint, and a test suite to keep software quality up.
Why making sure that there in a new-line at the end on .gitignore is more important than keeping out new warnings.

@refack
src: suppress orphan warnings
7f4a50b
@refack
src: fix C4800 implicit casting of int to bool
f91e496 
Conflicts:
	src/node.cc
@refack
src: fix remaining warnings
97ca146
@refack
deps: suppress known warnings
f4f2b6d 
And now that we have a warnings free build
we can activate "treat warnings as errors".

Note: the 'node' target still has two suppress warning types
@jasnell
Copy link
Member

jasnell commented Aug 13, 2015

@orangemocha ... do you see reason to keep this one open?

@refack refack closed this Aug 13, 2015
@orangemocha
Copy link
Contributor

Well, now we do have a CI that tests every commit. It would be nice to flush all warnings out and treat warnings as errors. This PR would have to be revisited though, and it would make more sense to do it in https://github.com/nodejs/node

@refack refack mentioned this pull request Oct 16, 2018
Merged
3 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
build pr v0.12
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@refack @trevnorris @indutny @orangemocha @jasnell @jbergstroem @Nodejs-Jenkins